Commit a6af1665 authored by Patrick Noland's avatar Patrick Noland Committed by Commit Bot

[ToolbarMVC] Replace LocationBar#setUrlToPageUrl with observer method

This removes the need to "poke" LocationBar when the url might have
changed; instead LocationBarModel is poked and notifies its observers.
LocationBarModel also triggers a URL change notification when a new tab
is selected. Following the established pattern, the url is calculated
lazily.

Bug: 1142883
Change-Id: Ic7b2b85343d482389082d896321cb42130ab758d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2507813
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Reviewed-by: default avatarPeter Conn <peconn@chromium.org>
Reviewed-by: default avatarFilip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822863}
parent 9b3d9815
......@@ -517,7 +517,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
protected void onConfigurationChanged(Configuration newConfig) {
super.onConfigurationChanged(newConfig);
mLocationBarModel.notifyTitleChanged();
mLocationBar.setUrlToPageUrl();
mLocationBarModel.notifyUrlChanged();
}
@Override
......@@ -703,7 +703,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
}
@Override
public void setUrlToPageUrl() {
public void onUrlChanged() {
Tab tab = getCurrentTab();
if (tab == null) {
mUrlCoordinator.setUrlBarData(
......@@ -766,7 +766,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
@Override
public void updateLoadingState(boolean updateUrl) {
if (updateUrl) setUrlToPageUrl();
if (updateUrl) onUrlChanged();
updateStatusIcon();
}
......@@ -776,7 +776,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
updateStatusIcon();
updateButtonsTint();
if (mUrlCoordinator.setUseDarkTextColors(mUseDarkColors)) {
setUrlToPageUrl();
onUrlChanged();
}
mTitleBar.setTextColor(ApiCompatibilityUtils.getColor(resources,
......@@ -814,7 +814,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
String contentDescription = getContext().getString(contentDescriptionId);
mSecurityButton.setContentDescription(contentDescription);
setUrlToPageUrl();
onUrlChanged();
mUrlBar.invalidate();
}
......
......@@ -30,15 +30,6 @@ public interface LocationBar extends Destroyable {
*/
void updateVisualsForState();
/**
* Sets the displayed URL to be the URL of the page currently showing.
*
* <p>The URL is converted to the most user friendly format (removing HTTP:// for example).
*
* <p>If the current tab is null, the URL text will be cleared.
*/
void setUrlToPageUrl();
/**
* Sets whether the location bar should have a layout showing a title.
*
......
......@@ -40,8 +40,9 @@ import java.util.List;
*
* <p>The coordinator creates and owns elements within this component.
*/
public final class LocationBarCoordinator
implements LocationBar, FakeboxDelegate, UrlBar.UrlBarDelegate, NativeInitObserver {
public final class LocationBarCoordinator implements LocationBar, FakeboxDelegate,
UrlBar.UrlBarDelegate, NativeInitObserver,
LocationBarDataProvider.Observer {
/** Identifies coordinators with methods specific to a device type. */
public interface SubCoordinator extends Destroyable {}
......@@ -49,6 +50,7 @@ public final class LocationBarCoordinator
@Nullable
private SubCoordinator mSubCoordinator;
private ActivityLifecycleDispatcher mActivityLifecycleDispatcher;
private LocationBarDataProvider mLocationbarDataProvider;
/**
* Creates {@link LocationBarCoordinator} and its subcoordinator: {@link
......@@ -97,7 +99,9 @@ public final class LocationBarCoordinator
throw new IllegalArgumentException(locationBarLayout.getClass().toString());
}
mLocationbarDataProvider = locationBarDataProvider;
mLocationBarLayout.setLocationBarDataProvider(locationBarDataProvider);
locationBarDataProvider.addObserver(this);
mLocationBarLayout.setProfileSupplier(profileObservableSupplier);
mLocationBarLayout.setDefaultTextEditActionModeCallback(actionModeCallback);
mLocationBarLayout.initializeControls(windowDelegate, windowAndroid, activityTabProvider,
......@@ -122,6 +126,10 @@ public final class LocationBarCoordinator
mLocationBarLayout.destroy();
mLocationBarLayout = null;
}
if (mLocationbarDataProvider != null) {
mLocationbarDataProvider.removeObserver(this);
mLocationbarDataProvider = null;
}
}
@Override
......@@ -144,11 +152,6 @@ public final class LocationBarCoordinator
mLocationBarLayout.updateVisualsForState();
}
@Override
public void setUrlToPageUrl() {
mLocationBarLayout.setUrlToPageUrl();
}
@Override
public void setShowTitle(boolean showTitle) {
mLocationBarLayout.setShowTitle(showTitle);
......@@ -260,6 +263,15 @@ public final class LocationBarCoordinator
return this;
}
// LocationBarDataObserver implementation
@Override
public void onTitleChanged() {}
@Override
public void onUrlChanged() {
mLocationBarLayout.setUrl(mLocationbarDataProvider.getCurrentUrl());
}
/**
* Returns the {@link LocationBarCoordinatorPhone} for this coordinator.
*
......
......@@ -32,7 +32,8 @@ public interface LocationBarDataProvider {
interface Observer {
void onTitleChanged();
// TODO(https://crbug.com/1139481): Add methods for other LocationBarDataProvider
// data, e.g. url, NTP, and security state.
// data, e.g. NTP and security state.
void onUrlChanged();
}
/** Adds an observer of changes to LocationBarDataProvider's data. */
......
......@@ -405,7 +405,7 @@ public class LocationBarLayout
public void revertChanges() {
if (!mUrlHasFocus) {
setUrlToPageUrl();
setUrl(mLocationBarDataProvider.getCurrentUrl());
} else {
String currentUrl = mLocationBarDataProvider.getCurrentUrl();
if (NativePageFactory.isNativePageUrl(
......@@ -472,7 +472,7 @@ public class LocationBarLayout
public void updateStatusIcon() {
mStatusCoordinator.updateStatusIcon();
// Update the URL in case the scheme change triggers a URL emphasis change.
setUrlToPageUrl();
setUrl(mLocationBarDataProvider.getCurrentUrl());
}
@Override
......@@ -579,7 +579,7 @@ public class LocationBarLayout
public void backKeyPressed() {
setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS);
// Revert the URL to match the current page.
setUrlToPageUrl();
setUrl(mLocationBarDataProvider.getCurrentUrl());
focusCurrentTab();
}
......@@ -594,7 +594,7 @@ public class LocationBarLayout
* @param updateUrl Whether to update the URL as a result of this call.
*/
public void updateLoadingState(boolean updateUrl) {
if (updateUrl) setUrlToPageUrl();
if (updateUrl) setUrl(mLocationBarDataProvider.getCurrentUrl());
mStatusCoordinator.updateStatusIcon();
}
......@@ -707,7 +707,7 @@ public class LocationBarLayout
// If the URL changed colors and is not focused, update the URL to account for the new
// color scheme.
if (mUrlCoordinator.setUseDarkTextColors(useDarkColors) && !mUrlBar.hasFocus()) {
setUrlToPageUrl();
setUrl(mLocationBarDataProvider.getCurrentUrl());
}
mStatusCoordinator.setUseDarkColors(useDarkColors);
......@@ -770,9 +770,7 @@ public class LocationBarLayout
*
* <p>If the current tab is null, the URL text will be cleared.
*/
public void setUrlToPageUrl() {
String currentUrl = mLocationBarDataProvider.getCurrentUrl();
protected void setUrl(String currentUrl) {
// If the URL is currently focused, do not replace the text they have entered with the URL.
// Once they stop editing the URL, the current tab's URL will automatically be filled in.
if (mUrlBar.hasFocus()) {
......@@ -1042,7 +1040,7 @@ public class LocationBarLayout
// Focus change caused by a close-tab may result in an invalid current tab.
if (mLocationBarDataProvider.hasTab()) {
setUrlToPageUrl();
setUrl(mLocationBarDataProvider.getCurrentUrl());
}
// Moving focus away from UrlBar(EditText) to a non-editable focus holder, such as
......@@ -1214,8 +1212,8 @@ public class LocationBarLayout
/**
* Changes the text on the url bar. The text update will be applied regardless of the current
* focus state (comparing to {@link #setUrlToPageUrl()} which only applies text updates when
* not focused).
* focus state (comparing to {@link #setUrlToPageUrl(mLocationBarDataProvider.getCurrentUrl())}
* which only applies text updates when not focused).
*
* @param urlBarData The contents of the URL bar, both for editing and displaying.
* @param scrollType Specifies how the text should be scrolled in the unfocused state.
......
......@@ -67,7 +67,7 @@ public class SearchActivityLocationBarLayout extends LocationBarLayout {
}
@Override
public void setUrlToPageUrl() {
protected void setUrl(String url) {
// Explicitly do nothing. The tab is invisible, so showing its URL would be confusing.
}
......
......@@ -114,6 +114,7 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
mIsIncognito = isIncognito;
updateUsingBrandColor();
notifyTitleChanged();
notifyUrlChanged();
}
@Override
......@@ -154,6 +155,12 @@ public class LocationBarModel implements ToolbarDataProvider, LocationBarDataPro
return getTab().getUrlString().trim();
}
public void notifyUrlChanged() {
for (LocationBarDataProvider.Observer observer : mLocationBarDataObservers) {
observer.onUrlChanged();
}
}
@Override
public NewTabPage getNewTabPageForCurrentTab() {
if (hasTab() && mTab.getNativePage() instanceof NewTabPage) {
......
......@@ -449,7 +449,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
assert tab == mLocationBarModel.getTab();
mLocationBar.updateStatusIcon();
mLocationBar.setUrlToPageUrl();
mLocationBarModel.notifyUrlChanged();
}
@Override
......@@ -544,7 +544,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
if (tab.getWebContents() != null
&& tab.getWebContents().getNavigationController() != null
&& tab.getWebContents().getNavigationController().isInitialNavigation()) {
mLocationBar.setUrlToPageUrl();
mLocationBarModel.notifyUrlChanged();
}
}
......@@ -1416,7 +1416,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
}
private void updateCurrentTabDisplayStatus() {
mLocationBar.setUrlToPageUrl();
mLocationBarModel.notifyUrlChanged();
updateTabLoadingState(true);
}
......
......@@ -209,7 +209,6 @@ public class TopToolbarCoordinator implements Toolbar {
mToolbarLayout.setTabModelSelector(mTabModelSelectorSupplier.get());
getLocationBar().updateVisualsForState();
getLocationBar().setUrlToPageUrl();
mToolbarLayout.setOnTabSwitcherClickHandler(tabSwitcherClickHandler);
mToolbarLayout.setOnTabSwitcherLongClickHandler(tabSwitcherLongClickHandler);
mToolbarLayout.setBookmarkClickHandler(bookmarkClickHandler);
......
......@@ -149,4 +149,22 @@ public class LocationBarModelTest {
regularLocationBarModel.notifyTitleChanged();
verify(mLocationBarDataObserver, times(2)).onTitleChanged();
}
@Test
@MediumTest
public void testObserversNotified_urlChange() {
LocationBarModel regularLocationBarModel = new TestRegularLocationBarModel(null);
regularLocationBarModel.addObserver(mLocationBarDataObserver);
verify(mLocationBarDataObserver, never()).onUrlChanged();
regularLocationBarModel.notifyUrlChanged();
verify(mLocationBarDataObserver).onUrlChanged();
regularLocationBarModel.setTab(mRegularTabMock, false);
verify(mLocationBarDataObserver, times(2)).onUrlChanged();
regularLocationBarModel.removeObserver(mLocationBarDataObserver);
regularLocationBarModel.notifyUrlChanged();
verify(mLocationBarDataObserver, times(2)).onUrlChanged();
}
}
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