New issue
Advanced search Search tips

Issue 746660 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Limit dependency between LocationBarLayout and currentTab

Project Member Reported by fgor...@chromium.org, Jul 19 2017

Issue description

Chrome Version: 61 and before
OS: Android


LocationBarLayout becomes more and more difficult to update, partially because it heavily depends on currentTab and not enough on its underlying data model: ToolbarDataProvider.

This bug is tracking the effort of limiting exposure of LocationBarLayout to current Tab.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34d00170603aca20269494a514b673cb1b462b8a

commit 34d00170603aca20269494a514b673cb1b462b8a
Author: Filip Gorski <fgorski@chromium.org>
Date: Thu Jul 27 16:28:34 2017

[Android Omnibox] Minor tweaks to limit exposure to current tab in LocationBarLayout

Updates ToolbarModelImpl with:
* Adding hasTab() method to ToolbarDataProvider interface
* Changing getCurrentUrl() to: guarantee it never returns null,
  but at least "" and making sure that returned URL is trimmed otherwise.
* Ensuring that it only ever uses its own getProfile() to
  get the profile, instead of grabbing it from alternative places (e.g. tab) 

Above changes make it possible to limit calls to get the tab in
LocationBarLayout:
* By calling mToolbarDataProvider.hasTab() (eliminates most null checks)
* Switching to calls to mToolbarDataProvider.getCurrentUrl(),
  mToolbarDataProvider.isIncognito(), mToolbarDataProvider.getProfile()

(This relies on presumption that tab.getProfile() matches profile on
toolbar data provider).

There are still places where getCurrentTab is called:
* Loading URL
* checking if we are dealing with offline page
* Getting the security level of the page loaded in tab
* Focusing current tab
* and when tab needs to be passed to other components,
  e.g. OmniboxPrerender, NativeTabFactory, PageInfoPopup

Bug: 746660
Change-Id: Iaab3fb0878a862f25adaf2dccd095b4d4fa0e890
Reviewed-on: https://chromium-review.googlesource.com/569064
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489963}
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarDataProvider.java
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
[modify] https://crrev.com/34d00170603aca20269494a514b673cb1b462b8a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarModelImpl.java

Sign in to add a comment