Commit b684c3c6 authored by Mia Glaese's avatar Mia Glaese Committed by Commit Bot

[Start Surface] Align Omnibox Suggestions with NTP

Problem description and design doc:
https://docs.google.com/document/d/1e1t3DgrtTkcluXz14xywwhJYq4isVfvVZbh1ZPvkL7M/edit?usp=sharing

Current:
Autocomplete uses url and page classification of current tab.
This leads to:
ZeroSuggestions not available on Start Surface (available on NTP).
Different suggestions for same input on StartSurface omnibox.
No omnibox suggestions if no tabs open.
Clank crashes on DCHECK in Debug mode if no tabs open.

Now:
Autocomplete uses NTP url for both StartSurface and NTP, even with no open tabs.
ZeroSuggestions available on start Surface.
Same Autocomplete suggestions for NTP and StartSurface.
No crashes in Debug mode.

Bug: 982018
Change-Id: I8de9b2e3822c5d08c71705466936245d6d0d44f3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894293
Commit-Queue: Mia Glaese <glamia@chromium.org>
Reviewed-by: default avatarPatrick Noland <pnoland@chromium.org>
Reviewed-by: default avatarYusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: default avatarGanggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718665}
parent 1ae42edd
...@@ -14,6 +14,7 @@ import android.view.WindowManager; ...@@ -14,6 +14,7 @@ import android.view.WindowManager;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.WindowDelegate; import org.chromium.chrome.browser.WindowDelegate;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.ntp.NewTabPage; import org.chromium.chrome.browser.ntp.NewTabPage;
import org.chromium.chrome.browser.omnibox.status.StatusView; import org.chromium.chrome.browser.omnibox.status.StatusView;
...@@ -372,6 +373,10 @@ public class LocationBarPhone extends LocationBarLayout { ...@@ -372,6 +373,10 @@ public class LocationBarPhone extends LocationBarLayout {
updateStatusVisibility(); updateStatusVisibility();
} }
public void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior) {
mAutocompleteCoordinator.setOverviewModeBehavior(overviewModeBehavior);
}
/** Update the status visibility according to the current state held in LocationBar. */ /** Update the status visibility according to the current state held in LocationBar. */
private void updateStatusVisibility() { private void updateStatusVisibility() {
boolean incognito = getToolbarDataProvider().isIncognito(); boolean incognito = getToolbarDataProvider().isIncognito();
......
...@@ -9,6 +9,7 @@ import android.view.KeyEvent; ...@@ -9,6 +9,7 @@ import android.view.KeyEvent;
import androidx.annotation.Nullable; import androidx.annotation.Nullable;
import org.chromium.chrome.browser.ActivityTabProvider; import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.omnibox.LocationBarVoiceRecognitionHandler; import org.chromium.chrome.browser.omnibox.LocationBarVoiceRecognitionHandler;
import org.chromium.chrome.browser.omnibox.UrlBar.UrlTextChangeListener; import org.chromium.chrome.browser.omnibox.UrlBar.UrlTextChangeListener;
import org.chromium.chrome.browser.omnibox.UrlFocusChangeListener; import org.chromium.chrome.browser.omnibox.UrlFocusChangeListener;
...@@ -33,6 +34,12 @@ public interface AutocompleteCoordinator extends UrlFocusChangeListener, UrlText ...@@ -33,6 +34,12 @@ public interface AutocompleteCoordinator extends UrlFocusChangeListener, UrlText
*/ */
void setToolbarDataProvider(ToolbarDataProvider toolbarDataProvider); void setToolbarDataProvider(ToolbarDataProvider toolbarDataProvider);
/**
* @param overviewModeBehavior A means of accessing the current OverviewModeState and a way to
* listen to state changes.
*/
void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior);
/** /**
* Updates the profile used for generating autocomplete suggestions. * Updates the profile used for generating autocomplete suggestions.
* @param profile The profile to be used. * @param profile The profile to be used.
......
...@@ -19,6 +19,7 @@ import org.chromium.base.Callback; ...@@ -19,6 +19,7 @@ import org.chromium.base.Callback;
import org.chromium.base.StrictModeContext; import org.chromium.base.StrictModeContext;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.ActivityTabProvider; import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.omnibox.LocationBarVoiceRecognitionHandler; import org.chromium.chrome.browser.omnibox.LocationBarVoiceRecognitionHandler;
import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider; import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController.OnSuggestionsReceivedListener; import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController.OnSuggestionsReceivedListener;
...@@ -172,6 +173,11 @@ public class AutocompleteCoordinatorImpl implements AutocompleteCoordinator { ...@@ -172,6 +173,11 @@ public class AutocompleteCoordinatorImpl implements AutocompleteCoordinator {
mMediator.setToolbarDataProvider(toolbarDataProvider); mMediator.setToolbarDataProvider(toolbarDataProvider);
} }
@Override
public void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior) {
mMediator.setOverviewModeBehavior(overviewModeBehavior);
}
@Override @Override
public void setAutocompleteProfile(Profile profile) { public void setAutocompleteProfile(Profile profile) {
mMediator.setAutocompleteProfile(profile); mMediator.setAutocompleteProfile(profile);
......
...@@ -27,6 +27,8 @@ import org.chromium.chrome.browser.ActivityTabProvider; ...@@ -27,6 +27,8 @@ import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.ActivityTabProvider.ActivityTabTabObserver; import org.chromium.chrome.browser.ActivityTabProvider.ActivityTabTabObserver;
import org.chromium.chrome.browser.ChromeFeatureList; import org.chromium.chrome.browser.ChromeFeatureList;
import org.chromium.chrome.browser.GlobalDiscardableReferencePool; import org.chromium.chrome.browser.GlobalDiscardableReferencePool;
import org.chromium.chrome.browser.compositor.layouts.EmptyOverviewModeObserver;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.image_fetcher.ImageFetcher; import org.chromium.chrome.browser.image_fetcher.ImageFetcher;
import org.chromium.chrome.browser.image_fetcher.ImageFetcherConfig; import org.chromium.chrome.browser.image_fetcher.ImageFetcherConfig;
import org.chromium.chrome.browser.image_fetcher.ImageFetcherFactory; import org.chromium.chrome.browser.image_fetcher.ImageFetcherFactory;
...@@ -111,6 +113,9 @@ class AutocompleteMediator ...@@ -111,6 +113,9 @@ class AutocompleteMediator
private final EntitySuggestionProcessor mEntitySuggestionProcessor; private final EntitySuggestionProcessor mEntitySuggestionProcessor;
private ToolbarDataProvider mDataProvider; private ToolbarDataProvider mDataProvider;
private OverviewModeBehavior mOverviewModeBehavior;
private OverviewModeBehavior.OverviewModeObserver mOverviewModeObserver;
private boolean mNativeInitialized; private boolean mNativeInitialized;
private AutocompleteController mAutocomplete; private AutocompleteController mAutocomplete;
private long mUrlFocusTime; private long mUrlFocusTime;
...@@ -188,6 +193,10 @@ class AutocompleteMediator ...@@ -188,6 +193,10 @@ class AutocompleteMediator
mImageFetcher.destroy(); mImageFetcher.destroy();
mImageFetcher = null; mImageFetcher = null;
} }
if (mOverviewModeObserver != null) {
mOverviewModeBehavior.removeOverviewModeObserver(mOverviewModeObserver);
mOverviewModeObserver = null;
}
} }
@Override @Override
...@@ -291,6 +300,25 @@ class AutocompleteMediator ...@@ -291,6 +300,25 @@ class AutocompleteMediator
mDataProvider = provider; mDataProvider = provider;
} }
/**
* @param overviewModeBehavior A means of accessing the current OverviewModeState and a way to
* listen to state changes.
*/
public void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior) {
assert overviewModeBehavior != null;
mOverviewModeBehavior = overviewModeBehavior;
mOverviewModeObserver = new EmptyOverviewModeObserver() {
@Override
public void onOverviewModeStartedShowing(boolean showToolbar) {
if (mDataProvider.shouldShowLocationBarInOverviewMode()) {
AutocompleteControllerJni.get().prefetchZeroSuggestResults();
}
}
};
mOverviewModeBehavior.addOverviewModeObserver(mOverviewModeObserver);
}
/** Set the WindowAndroid instance associated with the containing Activity. */ /** Set the WindowAndroid instance associated with the containing Activity. */
void setWindowAndroid(WindowAndroid window) { void setWindowAndroid(WindowAndroid window) {
if (mLifecycleDispatcher != null) { if (mLifecycleDispatcher != null) {
...@@ -775,7 +803,9 @@ class AutocompleteMediator ...@@ -775,7 +803,9 @@ class AutocompleteMediator
boolean preventAutocomplete = !mUrlBarEditingTextProvider.shouldAutocomplete(); boolean preventAutocomplete = !mUrlBarEditingTextProvider.shouldAutocomplete();
mRequestSuggestions = null; mRequestSuggestions = null;
if (!mDataProvider.hasTab()) { // There may be no tabs when searching form omnibox in overview mode. In that case,
// ToolbarDataProvider.getCurrentUrl() returns NTP url.
if (!mDataProvider.hasTab() && !mDataProvider.isInOverviewAndShowingOmnibox()) {
// crbug.com/764749 // crbug.com/764749
Log.w(TAG, "onTextChangedForAutocomplete: no tab"); Log.w(TAG, "onTextChangedForAutocomplete: no tab");
return; return;
...@@ -1003,14 +1033,16 @@ class AutocompleteMediator ...@@ -1003,14 +1033,16 @@ class AutocompleteMediator
* Make a zero suggest request if: * Make a zero suggest request if:
* - Native is loaded. * - Native is loaded.
* - The URL bar has focus. * - The URL bar has focus.
* - The current tab is not incognito. * - The the tab/overview is not incognito.
*/ */
private void startZeroSuggest() { private void startZeroSuggest() {
// Reset "edited" state in the omnibox if zero suggest is triggered -- new edits // Reset "edited" state in the omnibox if zero suggest is triggered -- new edits
// now count as a new session. // now count as a new session.
mHasStartedNewOmniboxEditSession = false; mHasStartedNewOmniboxEditSession = false;
mNewOmniboxEditSessionTimestamp = -1; mNewOmniboxEditSessionTimestamp = -1;
if (mNativeInitialized && mDelegate.isUrlBarFocused() && mDataProvider.hasTab()) {
if (mNativeInitialized && mDelegate.isUrlBarFocused()
&& (mDataProvider.hasTab() || mDataProvider.isInOverviewAndShowingOmnibox())) {
int pageClassification = int pageClassification =
mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox()); mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox());
mAutocomplete.startZeroSuggest(mDataProvider.getProfile(), mAutocomplete.startZeroSuggest(mDataProvider.getProfile(),
......
...@@ -121,6 +121,12 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope ...@@ -121,6 +121,12 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope
@Override @Override
public String getCurrentUrl() { public String getCurrentUrl() {
// Provide NTP url instead of most recent tab url for searches in overview mode (when Start
// Surface is enabled). .
if (isInOverviewAndShowingOmnibox()) {
return UrlConstants.NTP_URL;
}
// TODO(yusufo) : Consider using this for all calls from getTab() for accessing url. // TODO(yusufo) : Consider using this for all calls from getTab() for accessing url.
if (!hasTab()) return ""; if (!hasTab()) return "";
...@@ -283,7 +289,10 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope ...@@ -283,7 +289,10 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope
public Profile getProfile() { public Profile getProfile() {
Profile lastUsedProfile = Profile.getLastUsedProfile(); Profile lastUsedProfile = Profile.getLastUsedProfile();
if (mIsIncognito) { if (mIsIncognito) {
assert lastUsedProfile.hasOffTheRecordProfile(); // When in overview mode with no open tabs, there has not been created an
// OffTheRecordProfile yet. #getOffTheRecordProfile will create a profile if none
// exists.
assert lastUsedProfile.hasOffTheRecordProfile() || isInOverviewAndShowingOmnibox();
return lastUsedProfile.getOffTheRecordProfile(); return lastUsedProfile.getOffTheRecordProfile();
} }
return lastUsedProfile.getOriginalProfile(); return lastUsedProfile.getOriginalProfile();
...@@ -346,6 +355,11 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope ...@@ -346,6 +355,11 @@ public class LocationBarModel implements ToolbarDataProvider, ToolbarCommonPrope
@Override @Override
public int getPageClassification(boolean isFocusedFromFakebox) { public int getPageClassification(boolean isFocusedFromFakebox) {
if (mNativeLocationBarModelAndroid == 0) return 0; if (mNativeLocationBarModelAndroid == 0) return 0;
// Provide (NTP=1) as page class in overview mode (when Start Surface is enabled). No call
// to the backend necessary or possible, since there is no tab or navigation entry.
if (isInOverviewAndShowingOmnibox()) return 1;
return LocationBarModelJni.get().getPageClassification( return LocationBarModelJni.get().getPageClassification(
mNativeLocationBarModelAndroid, LocationBarModel.this, isFocusedFromFakebox); mNativeLocationBarModelAndroid, LocationBarModel.this, isFocusedFromFakebox);
} }
......
...@@ -968,7 +968,7 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF ...@@ -968,7 +968,7 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF
mToolbar.initializeWithNative(tabModelSelector, controlsVisibilityDelegate, layoutManager, mToolbar.initializeWithNative(tabModelSelector, controlsVisibilityDelegate, layoutManager,
tabSwitcherClickHandler, tabSwitcherLongClickHandler, newTabClickHandler, tabSwitcherClickHandler, tabSwitcherLongClickHandler, newTabClickHandler,
bookmarkClickHandler, customTabsBackClickHandler); bookmarkClickHandler, customTabsBackClickHandler, overviewModeBehavior);
mToolbar.addOnAttachStateChangeListener(new OnAttachStateChangeListener() { mToolbar.addOnAttachStateChangeListener(new OnAttachStateChangeListener() {
@Override @Override
......
...@@ -31,6 +31,7 @@ import org.chromium.chrome.browser.ThemeColorProvider.ThemeColorObserver; ...@@ -31,6 +31,7 @@ import org.chromium.chrome.browser.ThemeColorProvider.ThemeColorObserver;
import org.chromium.chrome.browser.ThemeColorProvider.TintObserver; import org.chromium.chrome.browser.ThemeColorProvider.TintObserver;
import org.chromium.chrome.browser.compositor.Invalidator; import org.chromium.chrome.browser.compositor.Invalidator;
import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost; import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.findinpage.FindToolbar; import org.chromium.chrome.browser.findinpage.FindToolbar;
import org.chromium.chrome.browser.fullscreen.BrowserStateBrowserControlsVisibilityDelegate; import org.chromium.chrome.browser.fullscreen.BrowserStateBrowserControlsVisibilityDelegate;
import org.chromium.chrome.browser.ntp.NewTabPage; import org.chromium.chrome.browser.ntp.NewTabPage;
...@@ -631,6 +632,8 @@ public abstract class ToolbarLayout ...@@ -631,6 +632,8 @@ public abstract class ToolbarLayout
void setLayoutUpdateHost(LayoutUpdateHost layoutUpdateHost) {} void setLayoutUpdateHost(LayoutUpdateHost layoutUpdateHost) {}
void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior) {}
/** /**
* @param attached Whether or not the web content is attached to the view heirarchy. * @param attached Whether or not the web content is attached to the view heirarchy.
*/ */
......
...@@ -52,6 +52,7 @@ import org.chromium.base.TraceEvent; ...@@ -52,6 +52,7 @@ import org.chromium.base.TraceEvent;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.compositor.Invalidator; import org.chromium.chrome.browser.compositor.Invalidator;
import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost; import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.device.DeviceClassManager; import org.chromium.chrome.browser.device.DeviceClassManager;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory; import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.flags.FeatureUtilities; import org.chromium.chrome.browser.flags.FeatureUtilities;
...@@ -1618,6 +1619,11 @@ public class ToolbarPhone extends ToolbarLayout implements Invalidator.Client, O ...@@ -1618,6 +1619,11 @@ public class ToolbarPhone extends ToolbarLayout implements Invalidator.Client, O
mLayoutUpdateHost = layoutUpdateHost; mLayoutUpdateHost = layoutUpdateHost;
} }
@Override
public void setOverviewModeBehavior(OverviewModeBehavior overviewModeBehavior) {
mLocationBar.setOverviewModeBehavior(overviewModeBehavior);
}
@Override @Override
public void finishAnimations() { public void finishAnimations() {
mClipRect = null; mClipRect = null;
......
...@@ -19,6 +19,7 @@ import org.chromium.chrome.R; ...@@ -19,6 +19,7 @@ import org.chromium.chrome.R;
import org.chromium.chrome.browser.ThemeColorProvider; import org.chromium.chrome.browser.ThemeColorProvider;
import org.chromium.chrome.browser.compositor.Invalidator; import org.chromium.chrome.browser.compositor.Invalidator;
import org.chromium.chrome.browser.compositor.layouts.LayoutManager; import org.chromium.chrome.browser.compositor.layouts.LayoutManager;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
import org.chromium.chrome.browser.findinpage.FindToolbar; import org.chromium.chrome.browser.findinpage.FindToolbar;
import org.chromium.chrome.browser.fullscreen.BrowserStateBrowserControlsVisibilityDelegate; import org.chromium.chrome.browser.fullscreen.BrowserStateBrowserControlsVisibilityDelegate;
import org.chromium.chrome.browser.omnibox.LocationBar; import org.chromium.chrome.browser.omnibox.LocationBar;
...@@ -120,7 +121,8 @@ public class TopToolbarCoordinator implements Toolbar { ...@@ -120,7 +121,8 @@ public class TopToolbarCoordinator implements Toolbar {
BrowserStateBrowserControlsVisibilityDelegate controlsVisibilityDelegate, BrowserStateBrowserControlsVisibilityDelegate controlsVisibilityDelegate,
LayoutManager layoutManager, OnClickListener tabSwitcherClickHandler, LayoutManager layoutManager, OnClickListener tabSwitcherClickHandler,
OnLongClickListener tabSwitcherLongClickHandler, OnClickListener newTabClickHandler, OnLongClickListener tabSwitcherLongClickHandler, OnClickListener newTabClickHandler,
OnClickListener bookmarkClickHandler, OnClickListener customTabsBackClickHandler) { OnClickListener bookmarkClickHandler, OnClickListener customTabsBackClickHandler,
OverviewModeBehavior overviewModeBehavior) {
if (mTabSwitcherModeCoordinatorPhone != null) { if (mTabSwitcherModeCoordinatorPhone != null) {
mTabSwitcherModeCoordinatorPhone.setOnTabSwitcherClickHandler(tabSwitcherClickHandler); mTabSwitcherModeCoordinatorPhone.setOnTabSwitcherClickHandler(tabSwitcherClickHandler);
mTabSwitcherModeCoordinatorPhone.setOnNewTabClickHandler(newTabClickHandler); mTabSwitcherModeCoordinatorPhone.setOnNewTabClickHandler(newTabClickHandler);
...@@ -136,6 +138,7 @@ public class TopToolbarCoordinator implements Toolbar { ...@@ -136,6 +138,7 @@ public class TopToolbarCoordinator implements Toolbar {
mToolbarLayout.setBookmarkClickHandler(bookmarkClickHandler); mToolbarLayout.setBookmarkClickHandler(bookmarkClickHandler);
mToolbarLayout.setCustomTabCloseClickHandler(customTabsBackClickHandler); mToolbarLayout.setCustomTabCloseClickHandler(customTabsBackClickHandler);
mToolbarLayout.setLayoutUpdateHost(layoutManager); mToolbarLayout.setLayoutUpdateHost(layoutManager);
mToolbarLayout.setOverviewModeBehavior(overviewModeBehavior);
mToolbarLayout.onNativeLibraryReady(); mToolbarLayout.onNativeLibraryReady();
} }
......
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