Skip to content

GitLab

T
tangled
Commit ce074ea4 authored by Patrick Noland's avatar Patrick Noland Committed by Chromium LUCI CQ
Browse files
Options

[ToolbarMVC] Extract revertChanges to LocationBarMediator 

This change moves the implementation of revertChanges to
LocationBarMediator. It also passes the call to selectAll directly to
the UrlBarCoordinator, and implements the OnKeyListener interface in
LocationBarCoordinator instead of LocationBarLayout. The latter change
is because this listener needs to be able to call revertChanges, which
LocationBarLayout can no longer do.

Bug: 1147581
Change-Id: I5df4ec31997ed072f0d6040b7d96e595471ec741
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552758
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: default avatarFilip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833781}
parent 8ec8c898
Hide whitespace changes
Inline Side-by-side
Showing with 138 additions and 149 deletions
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java
View file @ ce074ea4
......@@ -6,11 +6,9 @@ package org.chromium.chrome.browser.omnibox;
import android.view.ActionMode;
import android.view.View;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.core.view.ViewCompat;
import org.chromium.base.CallbackController;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
......@@ -61,6 +59,7 @@ public final class LocationBarCoordinator
private WindowDelegate mWindowDelegate;
private View mAutocompleteAnchorView;
private LocationBarMediator mLocationBarMediator;
private View mUrlBar;
private CallbackController mCallbackController = new CallbackController();
private boolean mNativeInitialized;
......@@ -113,14 +112,15 @@ public final class LocationBarCoordinator
new LocationBarCoordinatorTablet((LocationBarTablet) locationBarLayout);
}
View urlBar = mLocationBarLayout.findViewById(R.id.url_bar);
mUrlBar = mLocationBarLayout.findViewById(R.id.url_bar);
OneshotSupplierImpl<AssistantVoiceSearchService> assistantVoiceSearchSupplier =
new OneshotSupplierImpl();
mLocationBarMediator = new LocationBarMediator(
mLocationBarLayout, locationBarDataProvider, assistantVoiceSearchSupplier);
mUrlCoordinator = new UrlBarCoordinator((UrlBar) urlBar, windowDelegate, actionModeCallback,
mCallbackController.makeCancelable(mLocationBarMediator::onUrlFocusChange),
mLocationBarMediator);
mUrlCoordinator =
new UrlBarCoordinator((UrlBar) mUrlBar, windowDelegate, actionModeCallback,
mCallbackController.makeCancelable(mLocationBarMediator::onUrlFocusChange),
mLocationBarMediator);
mAutocompleteCoordinator =
new AutocompleteCoordinator(mLocationBarLayout, mLocationBarMediator, this,
mUrlCoordinator, activityLifecycleDispatcher, modalDialogManagerSupplier,
......@@ -128,8 +128,8 @@ public final class LocationBarCoordinator
StatusView statusView = mLocationBarLayout.findViewById(R.id.location_bar_status);
mStatusCoordinator = new StatusCoordinator(isTablet(), statusView, mUrlCoordinator,
incognitoStateProvider, modalDialogManagerSupplier, locationBarDataProvider);
mLocationBarMediator.setCoordinators(mStatusCoordinator);
mLocationBarMediator.setCoordinators(mStatusCoordinator, mAutocompleteCoordinator);
mUrlBar.setOnKeyListener(mLocationBarMediator);
mUrlCoordinator.addUrlTextChangeListener(mAutocompleteCoordinator);
// The LocationBar's direction is tied to the UrlBar's text direction. Icons inside the
......@@ -155,6 +155,8 @@ public final class LocationBarCoordinator
mSubCoordinator.destroy();
mSubCoordinator = null;
}
mUrlBar.setOnKeyListener(null);
mUrlBar = null;
mUrlCoordinator.destroy();
mUrlCoordinator = null;
mLocationBarLayout.removeUrlFocusChangeListener(mAutocompleteCoordinator);
......@@ -212,7 +214,7 @@ public final class LocationBarCoordinator
@Override
public void selectAll() {
mLocationBarMediator.selectAll();
mUrlCoordinator.selectAll();
}
@Override
......@@ -227,12 +229,12 @@ public final class LocationBarCoordinator
@Override
public View getContainerView() {
return mLocationBarMediator.getContainerView();
return mLocationBarLayout.getContainerView();
}
@Override
public View getSecurityIconView() {
return mLocationBarMediator.getSecurityIconView();
return mLocationBarLayout.getSecurityIconView();
}
/** Returns the {@link VoiceRecognitionHandler} associated with this LocationBar. */
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
View file @ ce074ea4
......@@ -22,7 +22,6 @@ import android.view.inputmethod.InputMethodManager;
import android.widget.FrameLayout;
import android.widget.ImageButton;
import android.widget.LinearLayout;
import android.widget.TextView;
import androidx.annotation.CallSuper;
import androidx.annotation.NonNull;
......@@ -43,7 +42,6 @@ import org.chromium.chrome.R;
import org.chromium.chrome.browser.WindowDelegate;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.locale.LocaleManager;
import org.chromium.chrome.browser.native_page.NativePageFactory;
import org.chromium.chrome.browser.ntp.NewTabPageUma;
import org.chromium.chrome.browser.omnibox.UrlBar.ScrollType;
import org.chromium.chrome.browser.omnibox.UrlBarCoordinator.SelectionState;
......@@ -58,7 +56,6 @@ import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.util.ChromeAccessibilityUtil;
import org.chromium.chrome.browser.util.KeyNavigationUtil;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.components.browser_ui.widget.CompositeTouchDelegate;
import org.chromium.components.embedder_support.util.UrlUtilities;
......@@ -131,45 +128,6 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
private TemplateUrlServiceObserver mTemplateUrlObserver;
private OverrideUrlLoadingDelegate mOverrideUrlLoadingDelegate;
/**
* Class to handle input from a hardware keyboard when the focus is on the URL bar. In
* particular, handle navigating the suggestions list from the keyboard.
*/
private final class UrlBarKeyListener implements OnKeyListener {
@Override
public boolean onKey(View v, int keyCode, KeyEvent event) {
boolean isRtl = v.getLayoutDirection() == LAYOUT_DIRECTION_RTL;
if (mAutocompleteCoordinator.handleKeyEvent(keyCode, event)) {
return true;
} else if (keyCode == KeyEvent.KEYCODE_BACK) {
if (KeyNavigationUtil.isActionDown(event) && event.getRepeatCount() == 0) {
// Tell the framework to start tracking this event.
getKeyDispatcherState().startTracking(event, this);
return true;
} else if (KeyNavigationUtil.isActionUp(event)) {
getKeyDispatcherState().handleUpEvent(event);
if (event.isTracking() && !event.isCanceled()) {
backKeyPressed();
return true;
}
}
} else if (keyCode == KeyEvent.KEYCODE_ESCAPE) {
if (KeyNavigationUtil.isActionDown(event) && event.getRepeatCount() == 0) {
revertChanges();
return true;
}
} else if ((!isRtl && KeyNavigationUtil.isGoRight(event))
|| (isRtl && KeyNavigationUtil.isGoLeft(event))) {
// Ensures URL bar doesn't lose focus, when RIGHT or LEFT (RTL) key is pressed while
// the cursor is positioned at the end of the text.
TextView tv = (TextView) v;
return tv.getSelectionStart() == tv.getSelectionEnd()
&& tv.getSelectionEnd() == tv.getText().length();
}
return false;
}
}
public LocationBarLayout(Context context, AttributeSet attrs) {
this(context, attrs, R.layout.location_bar);
......@@ -226,8 +184,6 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
StatusView statusView = findViewById(R.id.location_bar_status);
statusView.setCompositeTouchDelegate(mCompositeTouchDelegate);
mUrlBar.setOnKeyListener(new UrlBarKeyListener());
}
@Override
......@@ -343,26 +299,6 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS);
}
public void selectAll() {
mUrlCoordinator.selectAll();
}
public void revertChanges() {
if (!mUrlHasFocus) {
setUrl(mLocationBarDataProvider.getCurrentUrl());
} else {
String currentUrl = mLocationBarDataProvider.getCurrentUrl();
if (NativePageFactory.isNativePageUrl(
currentUrl, mLocationBarDataProvider.isIncognito())) {
setUrlBarTextEmpty();
} else {
setUrlBarText(mLocationBarDataProvider.getUrlBarData(), UrlBar.ScrollType.NO_SCROLL,
SelectionState.SELECT_ALL);
}
setKeyboardVisibility(false, false);
}
}
/* package */ void onUrlTextChanged() {
updateButtonVisibility();
}
......@@ -1130,7 +1066,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
* @param selectionState Specifies how the text should be selected in the focused state.
* @return Whether the URL was changed as a result of this call.
*/
private boolean setUrlBarText(
/* package */ boolean setUrlBarText(
UrlBarData urlBarData, @ScrollType int scrollType, @SelectionState int selectionState) {
return mUrlCoordinator.setUrlBarData(urlBarData, scrollType, selectionState);
}
......@@ -1139,7 +1075,7 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
* Clear any text in the URL bar.
* @return Whether this changed the existing text.
*/
private boolean setUrlBarTextEmpty() {
/* package */ boolean setUrlBarTextEmpty() {
boolean textChanged = mUrlCoordinator.setUrlBarData(
UrlBarData.EMPTY, UrlBar.ScrollType.SCROLL_TO_BEGINNING, SelectionState.SELECT_ALL);
forceOnTextChanged();
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java
View file @ ce074ea4
......@@ -5,7 +5,10 @@
package org.chromium.chrome.browser.omnibox;
import android.content.Context;
import android.view.KeyEvent;
import android.view.View;
import android.view.View.OnKeyListener;
import android.widget.TextView;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
......@@ -13,8 +16,10 @@ import androidx.annotation.Nullable;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.chrome.browser.AppHooks;
import org.chromium.chrome.browser.gsa.GSAState;
import org.chromium.chrome.browser.native_page.NativePageFactory;
import org.chromium.chrome.browser.ntp.FakeboxDelegate;
import org.chromium.chrome.browser.omnibox.UrlBar.UrlBarDelegate;
import org.chromium.chrome.browser.omnibox.UrlBarCoordinator.SelectionState;
import org.chromium.chrome.browser.omnibox.status.StatusCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteDelegate;
......@@ -22,6 +27,7 @@ import org.chromium.chrome.browser.omnibox.voice.AssistantVoiceSearchService;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler;
import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
import org.chromium.chrome.browser.util.KeyNavigationUtil;
import org.chromium.ui.base.WindowAndroid;
import java.util.List;
......@@ -30,16 +36,17 @@ import java.util.List;
* Mediator for the LocationBar component. Intended location for LocationBar business logic;
* currently, migration of this logic out of LocationBarLayout is in progress.
*/
class LocationBarMediator implements LocationBar, LocationBarDataProvider.Observer,
AutocompleteDelegate, FakeboxDelegate,
VoiceRecognitionHandler.Delegate,
AssistantVoiceSearchService.Observer, UrlBarDelegate {
private LocationBarLayout mLocationBarLayout;
class LocationBarMediator implements LocationBarDataProvider.Observer, AutocompleteDelegate,
FakeboxDelegate, VoiceRecognitionHandler.Delegate,
AssistantVoiceSearchService.Observer, UrlBarDelegate,
OnKeyListener {
private final LocationBarLayout mLocationBarLayout;
private VoiceRecognitionHandler mVoiceRecognitionHandler;
private LocationBarDataProvider mLocationBarDataProvider;
private final LocationBarDataProvider mLocationBarDataProvider;
private AssistantVoiceSearchService mAssistantVoiceSearchService;
private OneshotSupplierImpl<AssistantVoiceSearchService> mAssistantVoiceSearchSupplier;
private final OneshotSupplierImpl<AssistantVoiceSearchService> mAssistantVoiceSearchSupplier;
private StatusCoordinator mStatusCoordinator;
private AutocompleteCoordinator mAutocompleteCoordinator;
/*package */ LocationBarMediator(@NonNull LocationBarLayout locationBarLayout,
@NonNull LocationBarDataProvider locationBarDataProvider,
......@@ -52,6 +59,15 @@ class LocationBarMediator implements LocationBar, LocationBarDataProvider.Observ
mVoiceRecognitionHandler = new VoiceRecognitionHandler(this, mAssistantVoiceSearchSupplier);
}
/*package */ void destroy() {
if (mAssistantVoiceSearchService != null) {
mAssistantVoiceSearchService.destroy();
mAssistantVoiceSearchService = null;
}
mStatusCoordinator = null;
mAutocompleteCoordinator = null;
}
/*package */ void onUrlFocusChange(boolean hasFocus) {
mLocationBarLayout.onUrlFocusChange(hasFocus);
}
......@@ -84,92 +100,73 @@ class LocationBarMediator implements LocationBar, LocationBarDataProvider.Observ
* LocationBarMediator is a delegate for them, so is constructed beforehand.
*
* @param statusCoordinator
* @param autocompleteCoordinator
*/
/* package */ void setCoordinators(@NonNull StatusCoordinator statusCoordinator) {
/* package */ void setCoordinators(@NonNull StatusCoordinator statusCoordinator,
@NonNull AutocompleteCoordinator autocompleteCoordinator) {
mStatusCoordinator = statusCoordinator;
mAutocompleteCoordinator = autocompleteCoordinator;
}
// LocationBarData.Observer implementation
@Override
public void onIncognitoStateChanged() {
mLocationBarLayout.updateMicButtonState();
}
@Override
public void onNtpStartedLoading() {
mLocationBarLayout.onNtpStartedLoading();
}
@Override
public void onPrimaryColorChanged() {
/*package */ void updateVisualsForState() {
mLocationBarLayout.onPrimaryColorChanged();
}
@Override
public void onTitleChanged() {}
@Override
public void onUrlChanged() {
mLocationBarLayout.setUrl(mLocationBarDataProvider.getCurrentUrl());
/*package */ void setShowTitle(boolean showTitle) {
// This method is only used in CustomTabToolbar.
}
// LocationBar implementation.
@Override
public void destroy() {
mLocationBarLayout = null;
mVoiceRecognitionHandler = null;
if (mAssistantVoiceSearchService != null) {
mAssistantVoiceSearchService.destroy();
mAssistantVoiceSearchService = null;
}
mLocationBarDataProvider.removeObserver(this);
mLocationBarDataProvider = null;
mStatusCoordinator = null;
/*package */ void updateLoadingState(boolean updateUrl) {
mLocationBarLayout.updateLoadingState(updateUrl);
}
@Override
public void updateVisualsForState() {
mLocationBarLayout.onPrimaryColorChanged();
/*package */ void showUrlBarCursorWithoutFocusAnimations() {
mLocationBarLayout.showUrlBarCursorWithoutFocusAnimations();
}
@Override
public void setShowTitle(boolean showTitle) {
// This method is only used in CustomTabToolbar.
/*package */ void revertChanges() {
if (mLocationBarLayout.isUrlBarFocused()) {
String currentUrl = mLocationBarDataProvider.getCurrentUrl();
if (NativePageFactory.isNativePageUrl(
currentUrl, mLocationBarDataProvider.isIncognito())) {
mLocationBarLayout.setUrlBarTextEmpty();
} else {
mLocationBarLayout.setUrlBarText(mLocationBarDataProvider.getUrlBarData(),
UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_ALL);
}
setKeyboardVisibility(false, false);
} else {
mLocationBarLayout.setUrl(mLocationBarDataProvider.getCurrentUrl());
}
}
@Override
public void updateLoadingState(boolean updateUrl) {
mLocationBarLayout.updateLoadingState(updateUrl);
/*package */ void updateStatusIcon() {
mLocationBarLayout.updateStatusIcon();
}
@Override
public void showUrlBarCursorWithoutFocusAnimations() {
mLocationBarLayout.showUrlBarCursorWithoutFocusAnimations();
}
// LocationBarData.Observer implementation
@Override
public void selectAll() {
mLocationBarLayout.selectAll();
}
public void onTitleChanged() {}
@Override
public void revertChanges() {
mLocationBarLayout.revertChanges();
public void onUrlChanged() {
mLocationBarLayout.setUrl(mLocationBarDataProvider.getCurrentUrl());
}
@Override
public void updateStatusIcon() {
mLocationBarLayout.updateStatusIcon();
public void onIncognitoStateChanged() {
mLocationBarLayout.updateMicButtonState();
}
@Override
public View getContainerView() {
return mLocationBarLayout.getContainerView();
public void onNtpStartedLoading() {
mLocationBarLayout.onNtpStartedLoading();
}
@Override
public View getSecurityIconView() {
return mLocationBarLayout.getSecurityIconView();
public void onPrimaryColorChanged() {
mLocationBarLayout.onPrimaryColorChanged();
}
// FakeboxDelegate implementation.
......@@ -289,12 +286,6 @@ class LocationBarMediator implements LocationBar, LocationBarDataProvider.Observ
mLocationBarLayout.updateMicButtonState();
}
@Nullable
@Override
public FakeboxDelegate getFakeboxDelegate() {
return null;
}
@Override
public void setSearchQuery(String query) {
mLocationBarLayout.setSearchQuery(query);
......@@ -337,4 +328,38 @@ class LocationBarMediator implements LocationBar, LocationBarDataProvider.Observ
public void gestureDetected(boolean isLongPress) {
mLocationBarLayout.gestureDetected(isLongPress);
}
// OnKeyListener implementation.
@Override
public boolean onKey(View view, int keyCode, KeyEvent event) {
boolean isRtl = view.getLayoutDirection() == View.LAYOUT_DIRECTION_RTL;
if (mAutocompleteCoordinator.handleKeyEvent(keyCode, event)) {
return true;
} else if (keyCode == KeyEvent.KEYCODE_BACK) {
if (KeyNavigationUtil.isActionDown(event) && event.getRepeatCount() == 0) {
// Tell the framework to start tracking this event.
mLocationBarLayout.getKeyDispatcherState().startTracking(event, this);
return true;
} else if (KeyNavigationUtil.isActionUp(event)) {
mLocationBarLayout.getKeyDispatcherState().handleUpEvent(event);
if (event.isTracking() && !event.isCanceled()) {
mLocationBarLayout.backKeyPressed();
return true;
}
}
} else if (keyCode == KeyEvent.KEYCODE_ESCAPE) {
if (KeyNavigationUtil.isActionDown(event) && event.getRepeatCount() == 0) {
revertChanges();
return true;
}
} else if ((!isRtl && KeyNavigationUtil.isGoRight(event))
|| (isRtl && KeyNavigationUtil.isGoLeft(event))) {
// Ensures URL bar doesn't lose focus, when RIGHT or LEFT (RTL) key is pressed while
// the cursor is positioned at the end of the text.
TextView tv = (TextView) view;
return tv.getSelectionStart() == tv.getSelectionEnd()
&& tv.getSelectionEnd() == tv.getText().length();
}
return false;
}
}
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java
View file @ ce074ea4
......@@ -4,10 +4,9 @@
package org.chromium.chrome.browser.omnibox;
import static junit.framework.Assert.assertNull;
import static org.mockito.ArgumentMatchers.notNull;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import android.content.Context;
......@@ -25,9 +24,11 @@ import org.robolectric.annotation.Config;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.omnibox.UrlBarCoordinator.SelectionState;
import org.chromium.chrome.browser.omnibox.voice.AssistantVoiceSearchService;
import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.components.embedder_support.util.UrlConstants;
import org.chromium.components.search_engines.TemplateUrlService;
/** Unit tests for LocationBarMediator. */
......@@ -78,7 +79,7 @@ public class LocationBarMediatorTest {
public void testGetVoiceRecognitionHandler_safeToCallAfterDestroy() {
mMediator.onFinishNativeInitialization();
mMediator.destroy();
assertNull(mMediator.getVoiceRecognitionHandler());
mMediator.getVoiceRecognitionHandler();
}
@Test
......@@ -86,4 +87,29 @@ public class LocationBarMediatorTest {
mMediator.onNtpStartedLoading();
verify(mLocationBarLayout).onNtpStartedLoading();
}
@Test
public void testRevertChanges_focused() {
doReturn(true).when(mLocationBarLayout).isUrlBarFocused();
UrlBarData urlBarData = mock(UrlBarData.class);
doReturn(urlBarData).when(mLocationBarDataProvider).getUrlBarData();
mMediator.revertChanges();
verify(mLocationBarLayout)
.setUrlBarText(urlBarData, UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_ALL);
}
@Test
public void testRevertChanges_focusedNativePage() {
doReturn(UrlConstants.NTP_URL).when(mLocationBarDataProvider).getCurrentUrl();
doReturn(true).when(mLocationBarLayout).isUrlBarFocused();
mMediator.revertChanges();
verify(mLocationBarLayout).setUrlBarTextEmpty();
}
@Test
public void testRevertChanges_unFocused() {
doReturn("http://url.com").when(mLocationBarDataProvider).getCurrentUrl();
mMediator.revertChanges();
verify(mLocationBarLayout).setUrl("http://url.com");
}
}
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