Skip to content

GitLab

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

[ToolbarMVC] Move setUrlbarFocus to LocationBarMediator 

Bug: 1147581
Change-Id: If11ef6233939f1c58ffba34b4ab391f7665134d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596115
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Reviewed-by: default avatarFilip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844848}
parent 4e7fc9bc
Show whitespace changes
Inline Side-by-side
Showing with 119 additions and 68 deletions
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
View file @ 2ac8588e
......@@ -22,13 +22,11 @@ import android.widget.LinearLayout;
import androidx.annotation.CallSuper;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import androidx.core.view.MarginLayoutParamsCompat;
import org.chromium.base.ApiCompatibilityUtils;
import org.chromium.base.ObserverList;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.WindowDelegate;
......@@ -190,10 +188,18 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
return mUrlFocusedFromFakebox;
}
/* package */ void setUrlFocusedFromFakebox(boolean focusedFromFakebox) {
mUrlFocusedFromFakebox = focusedFromFakebox;
}
/* package */ boolean didFocusUrlFromQueryTiles() {
return mUrlFocusedFromQueryTiles;
}
/* package */ void setUrlFocusedFromQueryTiles(boolean focusedFromQueryTiles) {
mUrlFocusedFromQueryTiles = focusedFromQueryTiles;
}
/* package */ void setIsUrlFocusedWithoutAnimations(boolean isUrlFocusedWithoutAnimations) {
mUrlFocusedWithoutAnimations = isUrlFocusedWithoutAnimations;
}
......@@ -245,40 +251,6 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
}
}
/* package */ void setUrlBarFocus(
boolean shouldBeFocused, @Nullable String pastedText, @OmniboxFocusReason int reason) {
if (shouldBeFocused) {
if (!mUrlHasFocus) recordOmniboxFocusReason(reason);
if (reason == OmniboxFocusReason.FAKE_BOX_TAP
|| reason == OmniboxFocusReason.FAKE_BOX_LONG_PRESS
|| reason == OmniboxFocusReason.TASKS_SURFACE_FAKE_BOX_LONG_PRESS
|| reason == OmniboxFocusReason.TASKS_SURFACE_FAKE_BOX_TAP) {
mUrlFocusedFromFakebox = true;
}
if (reason == OmniboxFocusReason.QUERY_TILES_NTP_TAP) {
mUrlFocusedFromFakebox = true;
mUrlFocusedFromQueryTiles = true;
}
if (mUrlHasFocus && mUrlFocusedWithoutAnimations) {
handleUrlFocusAnimation(mUrlHasFocus);
} else {
mUrlBar.requestFocus();
}
} else {
assert pastedText == null;
mUrlBar.clearFocus();
}
if (pastedText != null) {
// This must be happen after requestUrlFocus(), which changes the selection.
mUrlCoordinator.setUrlBarData(UrlBarData.forNonUrlText(pastedText),
UrlBar.ScrollType.NO_SCROLL, UrlBarCoordinator.SelectionState.SELECT_END);
forceOnTextChanged();
}
}
/* package */ boolean isUrlBarFocused() {
return mUrlHasFocus;
}
......@@ -652,17 +624,12 @@ public class LocationBarLayout extends FrameLayout implements OnClickListener {
return mStatusCoordinator;
}
private void forceOnTextChanged() {
/* package */ void forceOnTextChanged() {
String textWithoutAutocomplete = mUrlCoordinator.getTextWithoutAutocomplete();
String textWithAutocomplete = mUrlCoordinator.getTextWithAutocomplete();
mAutocompleteCoordinator.onTextChanged(textWithoutAutocomplete, textWithAutocomplete);
}
/* package */ void recordOmniboxFocusReason(@OmniboxFocusReason int reason) {
RecordHistogram.recordEnumeratedHistogram(
"Android.OmniboxFocusReason", reason, OmniboxFocusReason.NUM_ENTRIES);
}
/**
* Handles any actions to be performed after all other actions triggered by the URL focus
* change. This will be called after any animations are performed to transition from one
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java
View file @ 2ac8588e
......@@ -19,6 +19,7 @@ import androidx.annotation.VisibleForTesting;
import org.chromium.base.CallbackController;
import org.chromium.base.CommandLine;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplier;
......@@ -448,6 +449,11 @@ class LocationBarMediator implements LocationBarDataProvider.Observer, FakeboxDe
}
}
private void recordOmniboxFocusReason(@OmniboxFocusReason int reason) {
RecordHistogram.recordEnumeratedHistogram(
"Android.OmniboxFocusReason", reason, OmniboxFocusReason.NUM_ENTRIES);
}
// LocationBarData.Observer implementation
// Using the default empty onSecurityStateChanged.
// Using the default empty onTitleChanged.
......@@ -492,7 +498,37 @@ class LocationBarMediator implements LocationBarDataProvider.Observer, FakeboxDe
@Override
public void setUrlBarFocus(boolean shouldBeFocused, @Nullable String pastedText, int reason) {
mLocationBarLayout.setUrlBarFocus(shouldBeFocused, pastedText, reason);
boolean urlHasFocus = mLocationBarLayout.isUrlBarFocused();
if (shouldBeFocused) {
if (!urlHasFocus) recordOmniboxFocusReason(reason);
if (reason == OmniboxFocusReason.FAKE_BOX_TAP
|| reason == OmniboxFocusReason.FAKE_BOX_LONG_PRESS
|| reason == OmniboxFocusReason.TASKS_SURFACE_FAKE_BOX_LONG_PRESS
|| reason == OmniboxFocusReason.TASKS_SURFACE_FAKE_BOX_TAP) {
mLocationBarLayout.setUrlFocusedFromFakebox(true);
}
if (reason == OmniboxFocusReason.QUERY_TILES_NTP_TAP) {
mLocationBarLayout.setUrlFocusedFromFakebox(true);
mLocationBarLayout.setUrlFocusedFromQueryTiles(true);
}
if (urlHasFocus && mLocationBarLayout.isUrlBarFocusedWithoutAnimations()) {
mLocationBarLayout.handleUrlFocusAnimation(true);
} else {
mUrlCoordinator.requestFocus();
}
} else {
assert pastedText == null;
mUrlCoordinator.clearFocus();
}
if (pastedText != null) {
// This must be happen after requestUrlFocus(), which changes the selection.
mUrlCoordinator.setUrlBarData(UrlBarData.forNonUrlText(pastedText),
UrlBar.ScrollType.NO_SCROLL, UrlBarCoordinator.SelectionState.SELECT_END);
mLocationBarLayout.forceOnTextChanged();
}
}
@Override
......@@ -569,7 +605,7 @@ class LocationBarMediator implements LocationBarDataProvider.Observer, FakeboxDe
// Ensure the UrlBar has focus before entering text. If the UrlBar is not focused,
// autocomplete text will be updated but the visible text will not.
mLocationBarLayout.setUrlBarFocus(
setUrlBarFocus(
/*shouldBeFocused=*/true, /*pastedText=*/null, OmniboxFocusReason.SEARCH_QUERY);
mLocationBarLayout.setUrlBarText(UrlBarData.forNonUrlText(query),
UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_ALL);
......@@ -621,8 +657,7 @@ class LocationBarMediator implements LocationBarDataProvider.Observer, FakeboxDe
@Override
public void gestureDetected(boolean isLongPress) {
mLocationBarLayout.recordOmniboxFocusReason(isLongPress
? OmniboxFocusReason.OMNIBOX_LONG_PRESS
recordOmniboxFocusReason(isLongPress ? OmniboxFocusReason.OMNIBOX_LONG_PRESS
: OmniboxFocusReason.OMNIBOX_TAP);
}
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBarCoordinator.java
View file @ 2ac8588e
......@@ -173,6 +173,14 @@ public class UrlBarCoordinator implements UrlBarEditingTextStateProvider, UrlFoc
return mUrlBar.hasFocus();
}
/* package */ void requestFocus() {
mUrlBar.requestFocus();
}
/* package */ void clearFocus() {
mUrlBar.clearFocus();
}
/**
* Controls keyboard visibility.
*
......
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/LocationBarLayoutTest.java
View file @ 2ac8588e
......@@ -496,11 +496,12 @@ public class LocationBarLayoutTest {
@SmallTest
public void testSetUrlBarFocus() {
final LocationBarLayout locationBar = getLocationBar();
LocationBarMediator locationBarMediator = getLocationBarMediator();
Assert.assertEquals(
0, RecordHistogram.getHistogramTotalCountForTesting("Android.OmniboxFocusReason"));
TestThreadUtils.runOnUiThreadBlocking(() -> {
locationBar.setUrlBarFocus(
locationBarMediator.setUrlBarFocus(
true, SEARCH_TERMS_URL, OmniboxFocusReason.FAKE_BOX_LONG_PRESS);
});
Assert.assertTrue(locationBar.isUrlBarFocused());
......@@ -510,7 +511,7 @@ public class LocationBarLayoutTest {
1, RecordHistogram.getHistogramTotalCountForTesting("Android.OmniboxFocusReason"));
TestThreadUtils.runOnUiThreadBlocking(() -> {
locationBar.setUrlBarFocus(true, SEARCH_TERMS, OmniboxFocusReason.SEARCH_QUERY);
locationBarMediator.setUrlBarFocus(true, SEARCH_TERMS, OmniboxFocusReason.SEARCH_QUERY);
});
Assert.assertTrue(locationBar.isUrlBarFocused());
Assert.assertTrue(locationBar.didFocusUrlFromFakebox());
......@@ -518,15 +519,17 @@ public class LocationBarLayoutTest {
Assert.assertEquals(
1, RecordHistogram.getHistogramTotalCountForTesting("Android.OmniboxFocusReason"));
TestThreadUtils.runOnUiThreadBlocking(
() -> { locationBar.setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS); });
TestThreadUtils.runOnUiThreadBlocking(() -> {
locationBarMediator.setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS);
});
Assert.assertFalse(locationBar.isUrlBarFocused());
Assert.assertFalse(locationBar.didFocusUrlFromFakebox());
Assert.assertEquals(
1, RecordHistogram.getHistogramTotalCountForTesting("Android.OmniboxFocusReason"));
TestThreadUtils.runOnUiThreadBlocking(
() -> { locationBar.setUrlBarFocus(true, null, OmniboxFocusReason.OMNIBOX_TAP); });
TestThreadUtils.runOnUiThreadBlocking(() -> {
locationBarMediator.setUrlBarFocus(true, null, OmniboxFocusReason.OMNIBOX_TAP);
});
Assert.assertTrue(locationBar.isUrlBarFocused());
Assert.assertFalse(locationBar.didFocusUrlFromFakebox());
Assert.assertEquals(
......
chrome/android/junit/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java
View file @ 2ac8588e
......@@ -11,7 +11,6 @@ import static junit.framework.Assert.assertNull;
import static junit.framework.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyObject;
......@@ -320,7 +319,7 @@ public class LocationBarMediatorTest {
mMediator.onFinishNativeInitialization();
mMediator.setSearchQuery(query);
verify(mLocationBarLayout).setUrlBarFocus(true, null, OmniboxFocusReason.SEARCH_QUERY);
verify(mUrlCoordinator).requestFocus();
verify(mLocationBarLayout)
.setUrlBarText(argThat(matchesUrlBarDataForQuery(query)),
eq(UrlBar.ScrollType.NO_SCROLL), eq(SelectionState.SELECT_ALL));
......@@ -331,14 +330,14 @@ public class LocationBarMediatorTest {
@Test
public void testSetSearchQuery_empty() {
mMediator.setSearchQuery("");
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
verify(mLocationBarLayout, never()).post(any());
mMediator.onFinishNativeInitialization();
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
mMediator.setSearchQuery("");
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
verify(mLocationBarLayout, never()).post(any());
}
......@@ -351,7 +350,7 @@ public class LocationBarMediatorTest {
verify(mLocationBarLayout).post(mRunnableCaptor.capture());
mRunnableCaptor.getValue().run();
verify(mLocationBarLayout).setUrlBarFocus(true, null, OmniboxFocusReason.SEARCH_QUERY);
verify(mUrlCoordinator).requestFocus();
verify(mLocationBarLayout)
.setUrlBarText(argThat(matchesUrlBarDataForQuery(query)),
eq(UrlBar.ScrollType.NO_SCROLL), eq(SelectionState.SELECT_ALL));
......@@ -379,14 +378,14 @@ public class LocationBarMediatorTest {
@Test
public void testPerformSearchQuery_empty() {
mMediator.performSearchQuery("", Collections.emptyList());
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
verify(mLocationBarLayout, never()).post(any());
mMediator.onFinishNativeInitialization();
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
mMediator.setSearchQuery("");
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).requestFocus();
verify(mLocationBarLayout, never()).post(any());
}
......@@ -398,7 +397,7 @@ public class LocationBarMediatorTest {
doReturn("").when(mTemplateUrlService).getUrlForSearchQuery("example search", params);
mMediator.performSearchQuery(query, params);
verify(mLocationBarLayout).setUrlBarFocus(true, null, OmniboxFocusReason.SEARCH_QUERY);
verify(mUrlCoordinator).requestFocus();
verify(mLocationBarLayout)
.setUrlBarText(argThat(matchesUrlBarDataForQuery(query)),
eq(UrlBar.ScrollType.NO_SCROLL), eq(SelectionState.SELECT_ALL));
......@@ -414,7 +413,7 @@ public class LocationBarMediatorTest {
newConfig.keyboard = Configuration.KEYBOARD_QWERTY;
mMediator.onConfigurationChanged(newConfig);
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).clearFocus();
}
@Test
......@@ -424,15 +423,15 @@ public class LocationBarMediatorTest {
Configuration newConfig = new Configuration();
newConfig.keyboard = Configuration.KEYBOARD_NOKEYS;
mMediator.onConfigurationChanged(newConfig);
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).clearFocus();
doReturn(true).when(mLocationBarLayout).isUrlBarFocused();
mMediator.onConfigurationChanged(newConfig);
verify(mLocationBarLayout, never()).setUrlBarFocus(anyBoolean(), anyString(), anyInt());
verify(mUrlCoordinator, never()).clearFocus();
doReturn(true).when(mLocationBarLayout).isUrlBarFocusedWithoutAnimations();
mMediator.onConfigurationChanged(newConfig);
verify(mLocationBarLayout).setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS);
verify(mUrlCoordinator).clearFocus();
}
@Test
......@@ -619,7 +618,46 @@ public class LocationBarMediatorTest {
doReturn(true).when(mLocationBarLayout).isUrlBarFocusedWithoutAnimations();
mMediator.setUrl(url, urlBarData);
verify(mLocationBarLayout).setUrlBarFocus(false, null, OmniboxFocusReason.UNFOCUS);
verify(mUrlCoordinator).clearFocus();
}
@Test
public void testSetUrlBarFocus_focusedFromFakebox() {
mMediator.setUrlBarFocus(true, null, OmniboxFocusReason.FAKE_BOX_TAP);
verify(mLocationBarLayout).setUrlFocusedFromFakebox(true);
verify(mUrlCoordinator).requestFocus();
}
@Test
public void testSetUrlBarFocus_focusedFromQueryTiles() {
mMediator.setUrlBarFocus(true, null, OmniboxFocusReason.QUERY_TILES_NTP_TAP);
verify(mLocationBarLayout).setUrlFocusedFromQueryTiles(true);
verify(mLocationBarLayout).setUrlFocusedFromFakebox(true);
verify(mUrlCoordinator).requestFocus();
}
@Test
public void testSetUrlBarFocus_triggersAnimation() {
doReturn(true).when(mLocationBarLayout).isUrlBarFocused();
doReturn(true).when(mLocationBarLayout).isUrlBarFocusedWithoutAnimations();
mMediator.setUrlBarFocus(true, null, OmniboxFocusReason.QUERY_TILES_NTP_TAP);
verify(mLocationBarLayout).handleUrlFocusAnimation(true);
}
@Test
public void testSetUrlBarFocus_notFocused() {
mMediator.setUrlBarFocus(false, null, OmniboxFocusReason.FAKE_BOX_TAP);
verify(mUrlCoordinator).clearFocus();
}
@Test
public void testSetUrlBarFocus_pastedText() {
mMediator.setUrlBarFocus(true, "pastedText", OmniboxFocusReason.OMNIBOX_TAP);
verify(mUrlCoordinator)
.setUrlBarData(argThat(matchesUrlBarDataForQuery("pastedText")),
eq(UrlBar.ScrollType.NO_SCROLL),
eq(UrlBarCoordinator.SelectionState.SELECT_END));
verify(mLocationBarLayout).forceOnTextChanged();
}
private ArgumentMatcher<UrlBarData> matchesUrlBarDataForQuery(String query) {
......
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