Commit 43554ac2 authored by Patrick Noland's avatar Patrick Noland Committed by Commit Bot

[ToolbarMVC] Add LocationBarDataObserver

This CL adds an observer interface to LocationBarDataProvider, allowing consumers to listen for changes. It also implements observer registration and notification in LocationBarModel.

Initially, only title change notifications are supported, with the intention to add more fields, e.g. url, in future changes. The corresponding LocationBar "poke" method, setTitleToPageTitle, is removed.

Bug: 1139481
Change-Id: I1240c8152c818a32761a18c846e6d11cf6efa4ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505800
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Reviewed-by: default avatarBrandon Wylie <wylieb@chromium.org>
Reviewed-by: default avatarFilip Gorski <fgorski@chromium.org>
Reviewed-by: default avatarPeter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822842}
parent a1d26a85
...@@ -58,8 +58,8 @@ import org.chromium.chrome.browser.page_info.ChromePageInfoControllerDelegate; ...@@ -58,8 +58,8 @@ import org.chromium.chrome.browser.page_info.ChromePageInfoControllerDelegate;
import org.chromium.chrome.browser.page_info.ChromePermissionParamsListBuilderDelegate; import org.chromium.chrome.browser.page_info.ChromePermissionParamsListBuilderDelegate;
import org.chromium.chrome.browser.tab.Tab; import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TrustedCdn; import org.chromium.chrome.browser.tab.TrustedCdn;
import org.chromium.chrome.browser.toolbar.LocationBarModel;
import org.chromium.chrome.browser.toolbar.ToolbarColors; import org.chromium.chrome.browser.toolbar.ToolbarColors;
import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
import org.chromium.chrome.browser.toolbar.top.ToolbarActionModeCallback; import org.chromium.chrome.browser.toolbar.top.ToolbarActionModeCallback;
import org.chromium.chrome.browser.toolbar.top.ToolbarLayout; import org.chromium.chrome.browser.toolbar.top.ToolbarLayout;
import org.chromium.chrome.browser.toolbar.top.ToolbarPhone; import org.chromium.chrome.browser.toolbar.top.ToolbarPhone;
...@@ -158,6 +158,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -158,6 +158,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
private String mFirstUrl; private String mFirstUrl;
private CustomTabLocationBar mLocationBar; private CustomTabLocationBar mLocationBar;
private LocationBarModel mLocationBarModel;
private Runnable mTitleAnimationStarter = new Runnable() { private Runnable mTitleAnimationStarter = new Runnable() {
@Override @Override
...@@ -246,13 +247,15 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -246,13 +247,15 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
} }
/** /**
* * Creates and returns a CustomTab-specific LocationBar. This also retains a reference to the
* @param locationBarDataProvider {@link ToolbarDataProvider} to be used for accessing Toolbar * passed LocationBarModel.
* @param locationBarModel {@link LocationBarModel} to be used for accessing LocationBar
* state. * state.
* @return The LocationBar implementation for this CustomTabToolbar. * @return The LocationBar implementation for this CustomTabToolbar.
*/ */
public LocationBar createLocationBar(LocationBarDataProvider locationBarDataProvider) { public LocationBar createLocationBar(LocationBarModel locationBarModel) {
mLocationBar = new CustomTabLocationBar(locationBarDataProvider); mLocationBarModel = locationBarModel;
mLocationBar = new CustomTabLocationBar(locationBarModel);
mUrlCoordinator.setDelegate(mLocationBar); mUrlCoordinator.setDelegate(mLocationBar);
mLocationBar.updateVisualsForState(); mLocationBar.updateVisualsForState();
return mLocationBar; return mLocationBar;
...@@ -358,7 +361,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -358,7 +361,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
@Override @Override
protected void onNavigatedToDifferentPage() { protected void onNavigatedToDifferentPage() {
super.onNavigatedToDifferentPage(); super.onNavigatedToDifferentPage();
mLocationBar.setTitleToPageTitle(); mLocationBarModel.notifyTitleChanged();
if (mState == STATE_TITLE_ONLY) { if (mState == STATE_TITLE_ONLY) {
if (TextUtils.isEmpty(mFirstUrl)) { if (TextUtils.isEmpty(mFirstUrl)) {
mFirstUrl = getToolbarDataProvider().getTab().getUrlString(); mFirstUrl = getToolbarDataProvider().getTab().getUrlString();
...@@ -513,7 +516,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -513,7 +516,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
@Override @Override
protected void onConfigurationChanged(Configuration newConfig) { protected void onConfigurationChanged(Configuration newConfig) {
super.onConfigurationChanged(newConfig); super.onConfigurationChanged(newConfig);
mLocationBar.setTitleToPageTitle(); mLocationBarModel.notifyTitleChanged();
mLocationBar.setUrlToPageUrl(); mLocationBar.setUrlToPageUrl();
} }
...@@ -624,11 +627,13 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -624,11 +627,13 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
/** /**
* Custom tab-specific implementation of the LocationBar interface. * Custom tab-specific implementation of the LocationBar interface.
*/ */
private class CustomTabLocationBar implements LocationBar, UrlBar.UrlBarDelegate { private class CustomTabLocationBar
implements LocationBar, UrlBar.UrlBarDelegate, LocationBarDataProvider.Observer {
private LocationBarDataProvider mLocationBarDataProvider; private LocationBarDataProvider mLocationBarDataProvider;
public CustomTabLocationBar(LocationBarDataProvider locationBarDataProvider) { public CustomTabLocationBar(LocationBarDataProvider locationBarDataProvider) {
mLocationBarDataProvider = locationBarDataProvider; mLocationBarDataProvider = locationBarDataProvider;
mLocationBarDataProvider.addObserver(this);
} }
public void onNativeLibraryReady() { public void onNativeLibraryReady() {
...@@ -676,7 +681,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -676,7 +681,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
} }
@Override @Override
public void setTitleToPageTitle() { public void onTitleChanged() {
String title = mLocationBarDataProvider.getTitle(); String title = mLocationBarDataProvider.getTitle();
if (!mLocationBarDataProvider.hasTab() || TextUtils.isEmpty(title)) { if (!mLocationBarDataProvider.hasTab() || TextUtils.isEmpty(title)) {
mTitleBar.setText(""); mTitleBar.setText("");
...@@ -710,7 +715,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -710,7 +715,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
String url = publisherUrl != null ? publisherUrl : tab.getUrlString().trim(); String url = publisherUrl != null ? publisherUrl : tab.getUrlString().trim();
if (mState == STATE_TITLE_ONLY) { if (mState == STATE_TITLE_ONLY) {
if (!TextUtils.isEmpty(mLocationBarDataProvider.getTitle())) { if (!TextUtils.isEmpty(mLocationBarDataProvider.getTitle())) {
setTitleToPageTitle(); onTitleChanged();
} }
} }
...@@ -829,7 +834,12 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL ...@@ -829,7 +834,12 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
} }
@Override @Override
public void destroy() {} public void destroy() {
if (mLocationBarDataProvider != null) {
mLocationBarDataProvider.removeObserver(this);
mLocationBarDataProvider = null;
}
}
@Override @Override
public void showUrlBarCursorWithoutFocusAnimations() {} public void showUrlBarCursorWithoutFocusAnimations() {}
......
...@@ -39,9 +39,6 @@ public interface LocationBar extends Destroyable { ...@@ -39,9 +39,6 @@ public interface LocationBar extends Destroyable {
*/ */
void setUrlToPageUrl(); void setUrlToPageUrl();
/** Sets the displayed title to the page title. */
void setTitleToPageTitle();
/** /**
* Sets whether the location bar should have a layout showing a title. * Sets whether the location bar should have a layout showing a title.
* *
......
...@@ -149,11 +149,6 @@ public final class LocationBarCoordinator ...@@ -149,11 +149,6 @@ public final class LocationBarCoordinator
mLocationBarLayout.setUrlToPageUrl(); mLocationBarLayout.setUrlToPageUrl();
} }
@Override
public void setTitleToPageTitle() {
mLocationBarLayout.setTitleToPageTitle();
}
@Override @Override
public void setShowTitle(boolean showTitle) { public void setShowTitle(boolean showTitle) {
mLocationBarLayout.setShowTitle(showTitle); mLocationBarLayout.setShowTitle(showTitle);
......
...@@ -23,6 +23,24 @@ import org.chromium.components.security_state.ConnectionSecurityLevel; ...@@ -23,6 +23,24 @@ import org.chromium.components.security_state.ConnectionSecurityLevel;
// TODO(crbug.com/1142887): Refine split between LocationBar properties and sub-component // TODO(crbug.com/1142887): Refine split between LocationBar properties and sub-component
// properties, e.g. security state, which is only used by the status icon. // properties, e.g. security state, which is only used by the status icon.
public interface LocationBarDataProvider { public interface LocationBarDataProvider {
/**
* Observer interface for consumers who wish to subscribe to updates of LocationBarData.
* Since LocationBarDataProvider data is typically calculated lazily, individual observer
* methods don't directly supply the updated value. Instead, the expectation is that the
* consumer will query the data it cares about.
*/
interface Observer {
void onTitleChanged();
// TODO(https://crbug.com/1139481): Add methods for other LocationBarDataProvider
// data, e.g. url, NTP, and security state.
}
/** Adds an observer of changes to LocationBarDataProvider's data. */
void addObserver(Observer observer);
/** Removes an observer of changes to LocationBarDataProvider's data. */
void removeObserver(Observer observer);
/** Returns The url for the currently active page.*/ /** Returns The url for the currently active page.*/
@NonNull @NonNull
String getCurrentUrl(); String getCurrentUrl();
......
...@@ -732,8 +732,6 @@ public class LocationBarLayout ...@@ -732,8 +732,6 @@ public class LocationBarLayout
return mStatusCoordinator.getSecurityIconView(); return mStatusCoordinator.getSecurityIconView();
} }
public void setTitleToPageTitle() {}
public void setShowTitle(boolean showTitle) {} public void setShowTitle(boolean showTitle) {}
@Override @Override
......
...@@ -99,6 +99,12 @@ class SearchBoxDataProvider implements LocationBarDataProvider { ...@@ -99,6 +99,12 @@ class SearchBoxDataProvider implements LocationBarDataProvider {
return false; return false;
} }
@Override
public void addObserver(Observer observer) {}
@Override
public void removeObserver(Observer observer) {}
@Override @Override
public String getCurrentUrl() { public String getCurrentUrl() {
return SearchWidgetProvider.getDefaultSearchEngineUrl(); return SearchWidgetProvider.getDefaultSearchEngineUrl();
......
...@@ -16,6 +16,7 @@ import androidx.annotation.Nullable; ...@@ -16,6 +16,7 @@ import androidx.annotation.Nullable;
import androidx.annotation.StringRes; import androidx.annotation.StringRes;
import androidx.annotation.VisibleForTesting; import androidx.annotation.VisibleForTesting;
import org.chromium.base.ObserverList;
import org.chromium.base.annotations.CalledByNative; import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods; import org.chromium.base.annotations.NativeMethods;
import org.chromium.chrome.R; import org.chromium.chrome.R;
...@@ -64,6 +65,8 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro ...@@ -64,6 +65,8 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
private boolean mShouldShowOmniboxInOverviewMode; private boolean mShouldShowOmniboxInOverviewMode;
private long mNativeLocationBarModelAndroid; private long mNativeLocationBarModelAndroid;
private ObserverList<LocationBarDataProvider.Observer> mLocationBarDataObservers =
new ObserverList<>();
/** /**
* Default constructor for this class. * Default constructor for this class.
...@@ -110,6 +113,7 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro ...@@ -110,6 +113,7 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
mTab = tab; mTab = tab;
mIsIncognito = isIncognito; mIsIncognito = isIncognito;
updateUsingBrandColor(); updateUsingBrandColor();
notifyTitleChanged();
} }
@Override @Override
...@@ -125,6 +129,16 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro ...@@ -125,6 +129,16 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
return mTab != null && mTab.isInitialized(); return mTab != null && mTab.isInitialized();
} }
@Override
public void addObserver(LocationBarDataProvider.Observer observer) {
mLocationBarDataObservers.addObserver(observer);
}
@Override
public void removeObserver(LocationBarDataProvider.Observer observer) {
mLocationBarDataObservers.removeObserver(observer);
}
@Override @Override
public String getCurrentUrl() { public String getCurrentUrl() {
// Provide NTP url instead of most recent tab url for searches in overview mode (when Start // Provide NTP url instead of most recent tab url for searches in overview mode (when Start
...@@ -254,6 +268,12 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro ...@@ -254,6 +268,12 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
return TextUtils.isEmpty(title) ? title : title.trim(); return TextUtils.isEmpty(title) ? title : title.trim();
} }
public void notifyTitleChanged() {
for (LocationBarDataProvider.Observer observer : mLocationBarDataObservers) {
observer.onTitleChanged();
}
}
@Override @Override
public boolean isIncognito() { public boolean isIncognito() {
return mIsIncognito; return mIsIncognito;
......
...@@ -454,7 +454,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve ...@@ -454,7 +454,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
@Override @Override
public void onTitleUpdated(Tab tab) { public void onTitleUpdated(Tab tab) {
mLocationBar.setTitleToPageTitle(); mLocationBarModel.notifyTitleChanged();
} }
@Override @Override
......
...@@ -210,6 +210,12 @@ public class VoiceRecognitionHandlerTest { ...@@ -210,6 +210,12 @@ public class VoiceRecognitionHandlerTest {
return false; return false;
} }
@Override
public void addObserver(Observer observer) {}
@Override
public void removeObserver(Observer observer) {}
@Override @Override
public String getCurrentUrl() { public String getCurrentUrl() {
return null; return null;
......
...@@ -4,6 +4,9 @@ ...@@ -4,6 +4,9 @@
package org.chromium.chrome.browser.toolbar; package org.chromium.chrome.browser.toolbar;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when; import static org.mockito.Mockito.when;
import androidx.test.filters.MediumTest; import androidx.test.filters.MediumTest;
...@@ -19,6 +22,7 @@ import org.mockito.MockitoAnnotations; ...@@ -19,6 +22,7 @@ import org.mockito.MockitoAnnotations;
import org.chromium.base.ContextUtils; import org.chromium.base.ContextUtils;
import org.chromium.base.test.BaseRobolectricTestRunner; import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.customtabs.CustomTabIncognitoManager; import org.chromium.chrome.browser.customtabs.CustomTabIncognitoManager;
import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
import org.chromium.chrome.browser.profiles.Profile; import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.tab.Tab; import org.chromium.chrome.browser.tab.Tab;
import org.chromium.ui.base.WindowAndroid; import org.chromium.ui.base.WindowAndroid;
...@@ -48,6 +52,8 @@ public class LocationBarModelTest { ...@@ -48,6 +52,8 @@ public class LocationBarModelTest {
@Mock @Mock
private Profile mNonPrimaryOTRProfileMock; private Profile mNonPrimaryOTRProfileMock;
@Mock
private LocationBarDataProvider.Observer mLocationBarDataObserver;
@Before @Before
public void setUp() { public void setUp() {
...@@ -125,4 +131,22 @@ public class LocationBarModelTest { ...@@ -125,4 +131,22 @@ public class LocationBarModelTest {
Profile profile = regularLocationBarModel.getProfile(); Profile profile = regularLocationBarModel.getProfile();
Assert.assertEquals(mRegularProfileMock, profile); Assert.assertEquals(mRegularProfileMock, profile);
} }
@Test
@MediumTest
public void testObserversNotified_titleChange() {
LocationBarModel regularLocationBarModel = new TestRegularLocationBarModel(null);
regularLocationBarModel.addObserver(mLocationBarDataObserver);
verify(mLocationBarDataObserver, never()).onTitleChanged();
regularLocationBarModel.notifyTitleChanged();
verify(mLocationBarDataObserver).onTitleChanged();
regularLocationBarModel.setTab(mRegularTabMock, false);
verify(mLocationBarDataObserver, times(2)).onTitleChanged();
regularLocationBarModel.removeObserver(mLocationBarDataObserver);
regularLocationBarModel.notifyTitleChanged();
verify(mLocationBarDataObserver, times(2)).onTitleChanged();
}
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment