Skip to content

GitLab

T
tangled
Commit 5fb3223e authored by Matthew Jones's avatar Matthew Jones Committed by Commit Bot
Browse files
Options

Simplify omnibox suggestions visibility logic 

This patch restricts the number of exposed methods for modifying the
visible state of the omnibox suggestions. Most of this logic is now
driven off of the focused state of the omnibox (via
UrlFocusChangeListener) and whether suggestions are available.

Change-Id: I2dad7eda0331f7e69ad49ebb9c89f71748f4bd6b
Reviewed-on: https://chromium-review.googlesource.com/c/1293991
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603539}
parent 8969733c
Hide whitespace changes
Inline Side-by-side
Showing with 45 additions and 143 deletions
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBar.java
View file @ 5fb3223e
......@@ -155,11 +155,6 @@ public interface LocationBar extends UrlBarDelegate {
*/
void updateMicButtonState();
/**
* Signal to hide the omnibox suggestions.
*/
void hideSuggestions();
/**
* Sets the callback to be used by default for text editing action bar.
* @param callback The callback to use.
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
View file @ 5fb3223e
......@@ -46,7 +46,6 @@ import org.chromium.chrome.browser.omnibox.UrlBar.ScrollType;
import org.chromium.chrome.browser.omnibox.UrlBarCoordinator.SelectionState;
import org.chromium.chrome.browser.omnibox.geo.GeolocationHeader;
import org.chromium.chrome.browser.omnibox.status.StatusViewCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator.AutocompleteDelegate;
import org.chromium.chrome.browser.omnibox.suggestions.OmniboxSuggestion;
......@@ -203,6 +202,7 @@ public class LocationBarLayout extends FrameLayout
};
mAutocompleteCoordinator =
new AutocompleteCoordinator(this, this, embedder, mUrlCoordinator);
addUrlFocusChangeListener(mAutocompleteCoordinator);
mUrlCoordinator.setUrlTextChangeListener(mAutocompleteCoordinator);
mMicButton = (AppCompatImageButton) findViewById(R.id.mic_button);
......@@ -350,6 +350,7 @@ public class LocationBarLayout extends FrameLayout
if (shouldBeFocused) {
mUrlBar.requestFocus();
} else {
hideKeyboard();
mUrlBar.clearFocus();
}
}
......@@ -376,7 +377,6 @@ public class LocationBarLayout extends FrameLayout
setUrlBarText(mToolbarDataProvider.getUrlBarData(), UrlBar.ScrollType.NO_SCROLL,
SelectionState.SELECT_ALL);
}
hideSuggestions();
hideKeyboard();
}
}
......@@ -464,8 +464,6 @@ public class LocationBarLayout extends FrameLayout
});
}
}
mAutocompleteCoordinator.onUrlFocusChanged(hasFocus);
}
/**
......@@ -478,7 +476,6 @@ public class LocationBarLayout extends FrameLayout
listener.onUrlFocusChange(hasFocus);
}
mAutocompleteCoordinator.maybeShowOmniboxResultsContainer();
updateFadingBackgroundView(hasFocus, false);
}
......@@ -697,19 +694,6 @@ public class LocationBarLayout extends FrameLayout
mDeleteButton.setVisibility(shouldShowDeleteButton() ? VISIBLE : GONE);
}
/**
* Hides the omnibox suggestion popup.
*
* <p>
* Signals the autocomplete controller to stop generating omnibox suggestions.
*
* @see AutocompleteController#stop(boolean)
*/
@Override
public final void hideSuggestions() {
mAutocompleteCoordinator.hideSuggestions();
}
@Override
public void onSuggestionsHidden() {
updateNavigationButton();
......@@ -808,13 +792,9 @@ public class LocationBarLayout extends FrameLayout
@Override
public void onClick(View v) {
if (v == mDeleteButton) {
if (!TextUtils.isEmpty(mUrlCoordinator.getTextWithAutocomplete())) {
setUrlBarTextEmpty();
hideSuggestions();
updateButtonVisibility();
}
setUrlBarTextEmpty();
updateButtonVisibility();
mAutocompleteCoordinator.startZeroSuggest();
RecordUserAction.record("MobileOmniboxDeleteUrl");
return;
} else if (v == mMicButton && mVoiceRecognitionHandler != null) {
......@@ -827,8 +807,6 @@ public class LocationBarLayout extends FrameLayout
@Override
public void backKeyPressed() {
setUrlBarFocus(false);
hideSuggestions();
hideKeyboard();
// Revert the URL to match the current page.
setUrlToPageUrl();
focusCurrentTab();
......@@ -911,8 +889,10 @@ public class LocationBarLayout extends FrameLayout
* @return Whether this changed the existing text.
*/
private boolean setUrlBarTextEmpty() {
return mUrlCoordinator.setUrlBarData(
boolean textChanged = mUrlCoordinator.setUrlBarData(
UrlBarData.EMPTY, UrlBar.ScrollType.SCROLL_TO_BEGINNING, SelectionState.SELECT_ALL);
mAutocompleteCoordinator.onTextChangedForAutocomplete();
return textChanged;
}
@Override
......@@ -963,9 +943,6 @@ public class LocationBarLayout extends FrameLayout
LocaleManager.getInstance().recordLocaleBasedSearchMetrics(false, url, transition);
focusCurrentTab();
// Prevent any upcoming omnibox suggestions from showing. We have to do this after we load
// the URL as this will hide the suggestions and trigger a cancel of the prerendered page.
mAutocompleteCoordinator.stopAutocomplete(true);
}
/**
......@@ -1015,7 +992,6 @@ public class LocationBarLayout extends FrameLayout
chromeActivity.addViewObscuringAllTabs(mScrim);
} else {
chromeActivity.removeViewObscuringAllTabs(mScrim);
mAutocompleteCoordinator.updateOmniboxResultsContainerVisibility(false);
}
}
......@@ -1066,23 +1042,6 @@ public class LocationBarLayout extends FrameLayout
mStatusViewCoordinator.setUnfocusedLocationBarWidth(unfocusedWidth);
}
@Override
public void onWindowFocusChanged(boolean hasWindowFocus) {
super.onWindowFocusChanged(hasWindowFocus);
if (!hasWindowFocus && !mAutocompleteCoordinator.isSuggestionModalShown()) {
hideSuggestions();
} else if (hasWindowFocus && mUrlHasFocus && mNativeInitialized) {
String currentUrlBarText = mUrlCoordinator.getTextWithAutocomplete();
if (TextUtils.isEmpty(currentUrlBarText)
|| TextUtils.equals(currentUrlBarText,
mToolbarDataProvider.getUrlBarData().getEditingOrDisplayText())) {
mAutocompleteCoordinator.startZeroSuggest();
} else {
mAutocompleteCoordinator.onTextChangedForAutocomplete();
}
}
}
@Override
protected void onWindowVisibilityChanged(int visibility) {
super.onWindowVisibilityChanged(visibility);
......
chrome/android/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java
View file @ 5fb3223e
......@@ -26,6 +26,7 @@ import org.chromium.chrome.R;
import org.chromium.chrome.browser.omnibox.OmniboxSuggestionType;
import org.chromium.chrome.browser.omnibox.UrlBar.UrlTextChangeListener;
import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
import org.chromium.chrome.browser.omnibox.UrlFocusChangeListener;
import org.chromium.chrome.browser.omnibox.VoiceSuggestionProvider.VoiceResult;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController.OnSuggestionsReceivedListener;
import org.chromium.chrome.browser.omnibox.suggestions.OmniboxResultsAdapter.OmniboxResultItem;
......@@ -46,7 +47,7 @@ import java.util.List;
* Coordinator that handles the interactions with the autocomplete system.
*/
public class AutocompleteCoordinator
implements OnSuggestionsReceivedListener, UrlTextChangeListener {
implements OnSuggestionsReceivedListener, UrlFocusChangeListener, UrlTextChangeListener {
private static final String TAG = "cr_Autocomplete";
// Delay triggering the omnibox results upon key press to allow the location bar to repaint
......@@ -178,6 +179,33 @@ public class AutocompleteCoordinator
mAutocomplete = new AutocompleteController(this);
}
@Override
public void onUrlFocusChange(boolean hasFocus) {
if (hasFocus) {
if (mNativeInitialized) {
startZeroSuggest();
} else {
mDeferredNativeRunnables.add(() -> {
if (TextUtils.isEmpty(mUrlBarEditingTextProvider.getTextWithAutocomplete())) {
startZeroSuggest();
}
});
}
maybeShowOmniboxResultsContainer();
} else {
// Prevent any upcoming omnibox suggestions from showing once a URL is loaded (and as
// a consequence the omnibox is unfocused).
stopAutocomplete(true);
updateOmniboxResultsContainerVisibility(false);
mHasStartedNewOmniboxEditSession = false;
mNewOmniboxEditSessionTimestamp = -1;
hideSuggestions();
mAnswersImageFetcher.clearCache();
}
}
/**
* Provides data and state for the toolbar component.
* @param toolbarDataProvider The data provider.
......@@ -321,7 +349,6 @@ public class AutocompleteCoordinator
updateSuggestionUrlIfNeeded(suggestion, position, false);
loadUrlFromOmniboxMatch(
suggestionMatchUrl, position, suggestion, mLastActionUpTimestamp);
hideSuggestions();
mDelegate.hideKeyboard();
}
......@@ -465,7 +492,7 @@ public class AutocompleteCoordinator
/**
* Conditionally show the omnibox suggestions container.
*/
public void maybeShowOmniboxResultsContainer() {
private void maybeShowOmniboxResultsContainer() {
if (isSuggestionsListShown() || mDelegate.isUrlBarFocused()) {
initOmniboxResultsContainer();
updateOmniboxResultsContainerVisibility(true);
......@@ -475,7 +502,7 @@ public class AutocompleteCoordinator
/**
* Update whether the omnibox suggestions container is visible.
*/
public void updateOmniboxResultsContainerVisibility(boolean visible) {
private void updateOmniboxResultsContainerVisibility(boolean visible) {
if (mOmniboxResultsContainer == null) return;
boolean currentlyVisible = mOmniboxResultsContainer.getVisibility() == View.VISIBLE;
......@@ -851,7 +878,7 @@ public class AutocompleteCoordinator
* - The URL bar has focus.
* - The current tab is not incognito.
*/
public void startZeroSuggest() {
private void startZeroSuggest() {
// hasWindowFocus() can return true before onWindowFocusChanged has been called, so this
// is an optimization, but not entirely reliable. The underlying controller needs to also
// ensure we do not double trigger zero query.
......@@ -917,7 +944,7 @@ public class AutocompleteCoordinator
*
* @see AutocompleteController#stop(boolean)
*/
public void hideSuggestions() {
private void hideSuggestions() {
if (mAutocomplete == null || !mNativeInitialized) return;
if (mShowSuggestions != null) mParent.removeCallbacks(mShowSuggestions);
......@@ -934,7 +961,7 @@ public class AutocompleteCoordinator
*
* @param clear Whether to clear the most recent autocomplete results.
*/
public void stopAutocomplete(boolean clear) {
private void stopAutocomplete(boolean clear) {
if (mAutocomplete != null) mAutocomplete.stop(clear);
cancelPendingAutocompleteStart();
}
......@@ -965,28 +992,6 @@ public class AutocompleteCoordinator
}
}
/**
* Notifies autocomplete that the URL focus state has changed.
*/
public void onUrlFocusChanged(boolean hasFocus) {
if (!hasFocus) {
mHasStartedNewOmniboxEditSession = false;
mNewOmniboxEditSessionTimestamp = -1;
hideSuggestions();
mAnswersImageFetcher.clearCache();
}
if (mNativeInitialized) {
startZeroSuggest();
} else {
mDeferredNativeRunnables.add(() -> {
if (TextUtils.isEmpty(mUrlBarEditingTextProvider.getTextWithAutocomplete())) {
startZeroSuggest();
}
});
}
}
/**
* Sets the autocomplete controller for the location bar.
*
......
chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
View file @ 5fb3223e
......@@ -775,9 +775,6 @@ public class CustomTabToolbar
@Override
public void revertChanges() {}
@Override
public void hideSuggestions() {}
@Override
public void updateMicButtonState() {}
......
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarLayout.java
View file @ 5fb3223e
......@@ -765,7 +765,7 @@ public abstract class ToolbarLayout extends FrameLayout implements Toolbar {
* @return Whether or not the current Tab did go back.
*/
protected boolean back() {
getLocationBar().hideSuggestions();
if (getLocationBar() != null) getLocationBar().setUrlBarFocus(false);
return mToolbarTabController != null ? mToolbarTabController.back() : false;
}
......@@ -774,7 +774,7 @@ public abstract class ToolbarLayout extends FrameLayout implements Toolbar {
* @return Whether or not the current Tab did go forward.
*/
protected boolean forward() {
getLocationBar().hideSuggestions();
if (getLocationBar() != null) getLocationBar().setUrlBarFocus(false);
return mToolbarTabController != null ? mToolbarTabController.forward() : false;
}
......@@ -785,7 +785,7 @@ public abstract class ToolbarLayout extends FrameLayout implements Toolbar {
* <p>The buttons of the toolbar will be updated as a result of making this call.
*/
protected void stopOrReloadCurrentTab() {
getLocationBar().hideSuggestions();
if (getLocationBar() != null) getLocationBar().setUrlBarFocus(false);
if (mToolbarTabController != null) mToolbarTabController.stopOrReloadCurrentTab();
}
......@@ -793,6 +793,7 @@ public abstract class ToolbarLayout extends FrameLayout implements Toolbar {
* Opens hompage in the current tab.
*/
protected void openHomepage() {
if (getLocationBar() != null) getLocationBar().setUrlBarFocus(false);
if (mToolbarTabController != null) mToolbarTabController.openHomepage();
}
......@@ -800,7 +801,6 @@ public abstract class ToolbarLayout extends FrameLayout implements Toolbar {
* Opens the Memex UI in the current tab.
*/
protected void openMemexUI() {
getLocationBar().hideSuggestions();
if (mToolbarTabController != null) mToolbarTabController.openMemexUI();
}
......
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
View file @ 5fb3223e
......@@ -1364,7 +1364,6 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
public void openHomepage() {
RecordUserAction.record("Home");
mLocationBar.hideSuggestions();
Tab currentTab = mToolbarModel.getTab();
if (currentTab == null) return;
String homePageUrl = HomepageManager.getHomepageUri();
......
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/OmniboxTest.java
View file @ 5fb3223e
......@@ -298,59 +298,6 @@ public class OmniboxTest {
}));
}
@Test
@MediumTest
@Feature("Omnibox")
public void testSuggestionsTriggeredOnWindowFocusGained() {
final LocationBarLayout locationBar =
(LocationBarLayout) mActivityTestRule.getActivity().findViewById(R.id.location_bar);
final UrlBar urlBar = (UrlBar) mActivityTestRule.getActivity().findViewById(R.id.url_bar);
OmniboxTestUtils.toggleUrlBarFocus(urlBar, true);
ThreadUtils.runOnUiThreadBlocking(() -> {
TestAutocompleteController controller = new TestAutocompleteController(locationBar,
sEmptySuggestionListener, new HashMap<String, List<SuggestionsResult>>());
locationBar.getAutocompleteCoordinator().setAutocompleteController(controller);
locationBar.onWindowFocusChanged(false);
locationBar.onWindowFocusChanged(true);
Assert.assertEquals("Zero suggest not triggered when URL focused but unchanged", 1,
controller.numZeroSuggestRequests());
});
ThreadUtils.runOnUiThreadBlocking(() -> {
urlBar.setText("");
TestAutocompleteController controller = new TestAutocompleteController(locationBar,
sEmptySuggestionListener, new HashMap<String, List<SuggestionsResult>>());
locationBar.getAutocompleteCoordinator().setAutocompleteController(controller);
locationBar.onWindowFocusChanged(false);
locationBar.onWindowFocusChanged(true);
Assert.assertEquals("Zero suggest not triggered when URL focused but empty", 1,
controller.numZeroSuggestRequests());
});
final TestAutocompleteController controller = new TestAutocompleteController(locationBar,
sEmptySuggestionListener, new HashMap<String, List<SuggestionsResult>>());
ThreadUtils.runOnUiThreadBlocking(() -> {
urlBar.setText("cows");
locationBar.getAutocompleteCoordinator().setAutocompleteController(controller);
locationBar.onWindowFocusChanged(false);
locationBar.onWindowFocusChanged(true);
Assert.assertEquals("Zero suggest incorrectly triggered when URL has changed", 0,
controller.numZeroSuggestRequests());
});
// Autocomplete is triggered async, so we need to poll to see that it is eventually
// requested.
CriteriaHelper.pollUiThread(Criteria.equals(true, new Callable<Boolean>() {
@Override
public Boolean call() throws Exception {
return controller.isStartAutocompleteCalled();
}
}));
}
// Sanity check that no text is displayed in the omnibox when on the NTP page and that the hint
// text is correct.
@Test
......
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