[omnibox] Scope the on-clobber ZeroSuggest flag to Contextual Web only
Originally, we added an on-clobber entrypoint to ZeroSuggest guarded by
a feature flag which applied to all page classifications.
On further thought, it seems best to make this feature flag apply to
the Contextual Web (OTHER) page classification only, as different page
classifications may have different UI entrypoints to ZeroSuggest.
Bug: 1106096
Change-Id: I376e0b22d1b23d202e729d7a133920d94d9708e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357209
Reviewed-by: Justin Donnelly <[email protected]>
Commit-Queue: Tommy Li <[email protected]>
Cr-Commit-Position: refs/heads/master@{#798330}
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 0d6118d..2977dfa 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -3692,10 +3692,12 @@
FEATURE_VALUE_TYPE(omnibox::kOmniboxTabSwitchSuggestions)},
#endif // defined(OS_ANDROID)
- {"omnibox-clobber-is-zero-suggest-entrypoint",
- flag_descriptions::kOmniboxClobberIsZeroSuggestEntrypointName,
- flag_descriptions::kOmniboxClobberIsZeroSuggestEntrypointDescription,
- kOsAll, FEATURE_VALUE_TYPE(omnibox::kClobberIsZeroSuggestEntrypoint)},
+ {"omnibox-clobber-triggers-contextual-web-zero-suggest",
+ flag_descriptions::kOmniboxClobberTriggersContextualWebZeroSuggestName,
+ flag_descriptions::
+ kOmniboxClobberTriggersContextualWebZeroSuggestDescription,
+ kOsAll,
+ FEATURE_VALUE_TYPE(omnibox::kClobberTriggersContextualWebZeroSuggest)},
{"omnibox-focus-gesture-triggers-contextual-web-zero-suggest",
flag_descriptions::
diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json
index 7cc1bf1..6033ff4 100644
--- a/chrome/browser/flag-metadata.json
+++ b/chrome/browser/flag-metadata.json
@@ -3246,7 +3246,7 @@
"expiry_milestone": 95
},
{
- "name": "omnibox-clobber-is-zero-suggest-entrypoint",
+ "name": "omnibox-clobber-triggers-contextual-web-zero-suggest",
"owners": [ "tommycli", "[email protected]" ],
"expiry_milestone": 95
},
diff --git a/chrome/browser/flag_descriptions.cc b/chrome/browser/flag_descriptions.cc
index 1f609954..60ffdcb 100644
--- a/chrome/browser/flag_descriptions.cc
+++ b/chrome/browser/flag_descriptions.cc
@@ -1480,14 +1480,12 @@
"Allows autocompleting bookmark, history, and document suggestions when the"
" user input is a prefix of their titles, as opposed to their URLs.";
-const char kOmniboxClobberIsZeroSuggestEntrypointName[] =
- "Omnibox Clobber is ZeroSuggest Entrypoint";
-const char kOmniboxClobberIsZeroSuggestEntrypointDescription[] =
+const char kOmniboxClobberTriggersContextualWebZeroSuggestName[] =
+ "Omnibox Clobber Triggers Contextual Web ZeroSuggest";
+const char kOmniboxClobberTriggersContextualWebZeroSuggestDescription[] =
"If enabled, when the user clears the whole omnibox text (i.e. via "
- "Backspace), Chrome will request ZeroSuggest suggestions. Note, this flag "
- "merely adds a new ZeroSuggest entrypoint. ZeroSuggest still must be "
- "enabled on the proper page classification (either by default or via a "
- "separate flag), or else this flag will do nothing.";
+ "Backspace), Chrome will request ZeroSuggest suggestions for the OTHER "
+ "page classification (contextual web).";
const char kOmniboxFocusGestureTriggersContextualWebZeroSuggestName[] =
"Omnibox Focus Gesture Triggers Contextual Web ZeroSuggest";
diff --git a/chrome/browser/flag_descriptions.h b/chrome/browser/flag_descriptions.h
index 60f35e1..be32407 100644
--- a/chrome/browser/flag_descriptions.h
+++ b/chrome/browser/flag_descriptions.h
@@ -868,8 +868,8 @@
extern const char kOmniboxAutocompleteTitlesName[];
extern const char kOmniboxAutocompleteTitlesDescription[];
-extern const char kOmniboxClobberIsZeroSuggestEntrypointName[];
-extern const char kOmniboxClobberIsZeroSuggestEntrypointDescription[];
+extern const char kOmniboxClobberTriggersContextualWebZeroSuggestName[];
+extern const char kOmniboxClobberTriggersContextualWebZeroSuggestDescription[];
extern const char kOmniboxFocusGestureTriggersContextualWebZeroSuggestName[];
extern const char
diff --git a/components/omnibox/browser/omnibox_edit_model.cc b/components/omnibox/browser/omnibox_edit_model.cc
index e798e6a..7da58e3e 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -480,11 +480,19 @@
return;
}
- if (changed_to_user_input_in_progress && user_text_.empty() &&
- base::FeatureList::IsEnabled(omnibox::kClobberIsZeroSuggestEntrypoint)) {
+ if (!is_keyword_selected() && changed_to_user_input_in_progress &&
+ user_text_.empty()) {
// In the case the user enters user-input-in-progress mode by clearing
- // everything (i.e. via Backspace), and the special flag is on, ask for
- // ZeroSuggestions instead of the normal prefix (as-you-type) autocomplete.
+ // everything (i.e. via Backspace), ask for ZeroSuggestions instead of the
+ // normal prefix (as-you-type) autocomplete.
+ //
+ // We also check that no keyword is selected, as otherwise that breaks
+ // entering keyword mode via Ctrl+K.
+ //
+ // TODO(tommycli): The difference between a ZeroSuggest request and a normal
+ // prefix autocomplete request is getting fuzzier, and should be fully
+ // encapsulated by the AutocompleteInput::focus_type() member. We should
+ // merge these two calls soon, lest we confuse future developers.
StartZeroSuggestRequest(/*user_clobbered_permanent_text=*/true);
} else {
// Otherwise run the normal prefix (as-you-type) autocomplete.
diff --git a/components/omnibox/browser/zero_suggest_provider.cc b/components/omnibox/browser/zero_suggest_provider.cc
index 1f0ac85..b586f75 100644
--- a/components/omnibox/browser/zero_suggest_provider.cc
+++ b/components/omnibox/browser/zero_suggest_provider.cc
@@ -624,16 +624,26 @@
return false;
}
- // When the omnibox is empty, only allow zero suggest for the ChromeOS
- // Launcher and NTP, unless the clobber flag is on.
- //
- // TODO(tommycli): Add more nuance here, likely with an omnibox_focus_type.
- if (input_type == metrics::OmniboxInputType::EMPTY &&
- !(page_class == metrics::OmniboxEventProto::CHROMEOS_APP_LIST ||
- IsNTPPage(page_class) ||
- base::FeatureList::IsEnabled(
- omnibox::kClobberIsZeroSuggestEntrypoint))) {
- return false;
+ if (input_type == metrics::OmniboxInputType::EMPTY) {
+ // Function that returns whether EMPTY input zero-suggest is allowed.
+ auto IsEmptyZeroSuggestAllowed = [&]() {
+ if (page_class == metrics::OmniboxEventProto::CHROMEOS_APP_LIST ||
+ IsNTPPage(page_class)) {
+ return true;
+ }
+
+ if (page_class == metrics::OmniboxEventProto::OTHER) {
+ return input.focus_type() == OmniboxFocusType::DELETED_PERMANENT_TEXT &&
+ base::FeatureList::IsEnabled(
+ omnibox::kClobberTriggersContextualWebZeroSuggest);
+ }
+
+ return false;
+ };
+
+ // Return false if disallowed. Otherwise, proceed down to further checks.
+ if (!IsEmptyZeroSuggestAllowed())
+ return false;
}
// When omnibox contains pre-populated content, only show zero suggest for
diff --git a/components/omnibox/browser/zero_suggest_provider_unittest.cc b/components/omnibox/browser/zero_suggest_provider_unittest.cc
index 53f7223..33d15c4f 100644
--- a/components/omnibox/browser/zero_suggest_provider_unittest.cc
+++ b/components/omnibox/browser/zero_suggest_provider_unittest.cc
@@ -237,10 +237,21 @@
// Enable on-clobber in addition to on-focus.
{
base::test::ScopedFeatureList features;
- features.InitAndEnableFeature(omnibox::kClobberIsZeroSuggestEntrypoint);
+ features.InitAndEnableFeature(
+ omnibox::kClobberTriggersContextualWebZeroSuggest);
EXPECT_FALSE(provider_->AllowZeroSuggestSuggestions(prefix_input));
EXPECT_TRUE(provider_->AllowZeroSuggestSuggestions(on_focus_input));
EXPECT_TRUE(provider_->AllowZeroSuggestSuggestions(on_clobber_input));
+
+ // Sanity check that we only affect the OTHER page classification.
+ AutocompleteInput on_clobber_serp(
+ base::string16(),
+ metrics::OmniboxEventProto::
+ SEARCH_RESULT_PAGE_DOING_SEARCH_TERM_REPLACEMENT,
+ TestSchemeClassifier());
+ on_clobber_serp.set_current_url(GURL(input_url));
+ on_clobber_serp.set_focus_type(OmniboxFocusType::DELETED_PERMANENT_TEXT);
+ EXPECT_FALSE(provider_->AllowZeroSuggestSuggestions(on_clobber_serp));
}
// Disable on-focus.
@@ -266,7 +277,7 @@
{
base::test::ScopedFeatureList features;
features.InitWithFeatures(
- {omnibox::kClobberIsZeroSuggestEntrypoint},
+ {omnibox::kClobberTriggersContextualWebZeroSuggest},
{omnibox::kFocusGestureTriggersContextualWebZeroSuggest});
EXPECT_FALSE(provider_->AllowZeroSuggestSuggestions(prefix_input));
EXPECT_FALSE(provider_->AllowZeroSuggestSuggestions(on_focus_input));
diff --git a/components/omnibox/common/omnibox_features.cc b/components/omnibox/common/omnibox_features.cc
index 5161af4..6fe6155 100644
--- a/components/omnibox/common/omnibox_features.cc
+++ b/components/omnibox/common/omnibox_features.cc
@@ -213,12 +213,11 @@
base::FEATURE_DISABLED_BY_DEFAULT};
// If enabled, when the user clears the whole omnibox text (i.e. via Backspace),
-// Chrome will request ZeroSuggest suggestions. Note, this flag merely adds a
-// new ZeroSuggest entrypoint. ZeroSuggest still must be enabled on the proper
-// page classification (either by default or via a separate flag), or else this
-// flag will do nothing.
-const base::Feature kClobberIsZeroSuggestEntrypoint{
- "OmniboxClobberIsZeroSuggestEntrypoint", base::FEATURE_DISABLED_BY_DEFAULT};
+// Chrome will request remote ZeroSuggest suggestions for the OTHER page
+// classification (contextual web).
+const base::Feature kClobberTriggersContextualWebZeroSuggest{
+ "OmniboxClobberTriggersContextualWebZeroSuggest",
+ base::FEATURE_DISABLED_BY_DEFAULT};
// Disable this flag to prevent focus gestures (e.g. clicks, taps, Ctrl+L) from
// triggering ZeroSuggest for the OTHER page classification (contextual web).
diff --git a/components/omnibox/common/omnibox_features.h b/components/omnibox/common/omnibox_features.h
index d3726bfe..858ec9f 100644
--- a/components/omnibox/common/omnibox_features.h
+++ b/components/omnibox/common/omnibox_features.h
@@ -53,7 +53,7 @@
extern const base::Feature kDynamicMaxAutocomplete;
// On-Focus Suggestions a.k.a. ZeroSuggest.
-extern const base::Feature kClobberIsZeroSuggestEntrypoint;
+extern const base::Feature kClobberTriggersContextualWebZeroSuggest;
extern const base::Feature kFocusGestureTriggersContextualWebZeroSuggest;
extern const base::Feature kOmniboxLocalZeroSuggestAgeThreshold;
extern const base::Feature kOmniboxLocalZeroSuggestFrecencyRanking;
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index f989efe..40912339 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -41542,6 +41542,8 @@
<int value="-538141684" label="SafetyTip:enabled"/>
<int value="-536289234" label="ssl-interstitial-v2-colorful"/>
<int value="-536230323" label="SingleClickAutofill:disabled"/>
+ <int value="-536003711"
+ label="OmniboxClobberTriggersContextualWebZeroSuggest:enabled"/>
<int value="-535662704" label="BundledConnectionHelp:enabled"/>
<int value="-535208779" label="enable-native-cups"/>
<int value="-534470003" label="OmniboxOnDeviceHeadProviderIncognito:enabled"/>
@@ -43022,6 +43024,8 @@
<int value="1057014709" label="EnforceTLS13Downgrade:enabled"/>
<int value="1057887829" label="AutofillScanThemeDialog:disabled"/>
<int value="1059007599" label="enable-gpu-appcontainer"/>
+ <int value="1059283439"
+ label="OmniboxClobberTriggersContextualWebZeroSuggest:disabled"/>
<int value="1059698271" label="EnableZeroStateSuggestions:disabled"/>
<int value="1060319397" label="enable-data-reduction-proxy-carrier-test"/>
<int value="1060780557" label="DynamicColorGamut:disabled"/>
