[M136] [omnibox][zps] Disallow zero-prefix suggest for ineligible inputs.
Original change's description:
> [omnibox][zps] Disallow zero-prefix suggest for ineligible inputs.
>
> During the course of testing "On-focus ZPS on SRP/Web", it was found
> that a multi-edit clobber of the Omnibox (i.e. backspacing characters
> one-by-one until the text is empty) was resulting in zero-prefix
> suggestions being requested by ZeroSuggestProvider (even though the
> input was technically NOT eligible for zero-prefix suggest).
>
> In particular, backspacing the text (in the Omnibox)
> character-by-character until it's empty will result in an
> AutocompleteInput that has the EMPTY input type with a DEFAULT focus
> type (due to user input being in-progress). Given that a zero-prefix
> input is defined as any AutocompleteInput with a non-DEFAULT focus type
> (i.e. either FOCUS or CLOBBER), this kind of scenario technically should
> NOT trigger zero-prefix suggest.
>
> However, due to the recent removal of focus type checks in
> http://crrev.com/c/5976962, this kind of input was bypassing the
> relevant checks and triggering zero-prefix suggest. This CL essentially
> adds an "early exit" condition which considers any input with DEFAULT
> focus type (i.e. non-zero-prefix input) as ineligible for zero-prefix
> suggest.
>
> Bug: 406498945
> Change-Id: I6d99cc7da2a11b2f1f935bd82266dab6192c78ee
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6441623
> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
> Commit-Queue: Khalid Peer <khalidpeer@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1445396}
Bug: 410025936,406498945
Change-Id: I6d99cc7da2a11b2f1f935bd82266dab6192c78ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454985
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Chrome Cherry Picker <chrome-cherry-picker@chops-service-accounts.iam.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/7103@{#871}
Cr-Branched-From: e09430c64983fc906f37a9f7e6806275c9b67b86-refs/heads/main@{#1440670}
diff --git a/components/omnibox/browser/zero_suggest_provider.cc b/components/omnibox/browser/zero_suggest_provider.cc
index 3517569c..270378aa 100644
--- a/components/omnibox/browser/zero_suggest_provider.cc
+++ b/components/omnibox/browser/zero_suggest_provider.cc
@@ -236,28 +236,27 @@
// are done in GetResultTypeAndEligibility().
ResultType ResultTypeForInput(const AutocompleteInput& input) {
const auto page_class = input.current_page_classification();
- const auto focus_type_input_type =
- std::make_pair(input.focus_type(), input.type());
+
+ // Disallow non-zero-prefix inputs.
+ if (!input.IsZeroSuggest()) {
+ return ResultType::kNone;
+ }
// Android Search Widget.
if (page_class == OEP::ANDROID_SHORTCUTS_WIDGET) {
- if (focus_type_input_type.first != OFT::INTERACTION_DEFAULT) {
- return ResultType::kRemoteNoURL;
- }
+ return ResultType::kRemoteNoURL;
}
// New Tab Page.
if (omnibox::IsNTPPage(page_class)) {
- if (focus_type_input_type ==
- std::make_pair(OFT::INTERACTION_FOCUS, OIT::EMPTY)) {
+ if (input.type() == OIT::EMPTY) {
return ResultType::kRemoteNoURL;
}
}
// Lens unimodal, multimodal, and contextual searchboxes.
if (omnibox::IsLensSearchbox(page_class)) {
- if (focus_type_input_type ==
- std::make_pair(OFT::INTERACTION_FOCUS, OIT::EMPTY)) {
+ if (input.type() == OIT::EMPTY) {
return ResultType::kRemoteNoURL;
}
}
@@ -272,12 +271,12 @@
// Open Web and Search Results Page.
if (omnibox::IsOtherWebPage(page_class) ||
omnibox::IsSearchResultsPage(page_class)) {
- if (focus_type_input_type.second == OIT::URL &&
+ if (input.type() == OIT::URL &&
(is_ios || base::FeatureList::IsEnabled(
omnibox::kFocusTriggersWebAndSRPZeroSuggest))) {
return ResultType::kRemoteSendURL;
}
- if (focus_type_input_type.second == OIT::EMPTY && !is_ios) {
+ if (input.type() == OIT::EMPTY && !is_ios) {
return ResultType::kRemoteSendURL;
}
}
diff --git a/components/omnibox/browser/zero_suggest_provider_unittest.cc b/components/omnibox/browser/zero_suggest_provider_unittest.cc
index af3bc851..8090b7f 100644
--- a/components/omnibox/browser/zero_suggest_provider_unittest.cc
+++ b/components/omnibox/browser/zero_suggest_provider_unittest.cc
@@ -154,13 +154,17 @@
}
// An AutocompleteInput that gets Zero Prefix Suggestions on NTP.
- AutocompleteInput ZeroPrefixInputForNTP(const bool is_prefetch) {
+ AutocompleteInput ZeroPrefixInputForNTP(
+ const bool is_prefetch,
+ const bool user_input_in_progress = false) {
AutocompleteInput input(u"",
is_prefetch
? metrics::OmniboxEventProto::NTP_ZPS_PREFETCH
: metrics::OmniboxEventProto::NTP_REALBOX,
TestSchemeClassifier());
- input.set_focus_type(metrics::OmniboxFocusType::INTERACTION_FOCUS);
+ input.set_focus_type(user_input_in_progress
+ ? metrics::OmniboxFocusType::INTERACTION_DEFAULT
+ : metrics::OmniboxFocusType::INTERACTION_FOCUS);
return input;
}
@@ -175,6 +179,7 @@
// An AutocompleteInput that gets Zero Prefix Suggestions on WEB.
AutocompleteInput ZeroPrefixInputForWeb(
const bool is_prefetch,
+ const bool user_input_in_progress = false,
const std::string& input_url = "https://example.com/") {
// On IOS WEB/SRP, input text is not empty.
AutocompleteInput input(is_ios ? base::ASCIIToUTF16(input_url) : u"",
@@ -183,23 +188,9 @@
: metrics::OmniboxEventProto::OTHER,
TestSchemeClassifier());
input.set_current_url(GURL(input_url));
- input.set_focus_type(metrics::OmniboxFocusType::INTERACTION_FOCUS);
- return input;
- }
-
- // An AutocompleteInput that gets Zero Prefix Suggestions on SRP.
- AutocompleteInput ZeroPrefixInputForSRP(
- const bool is_prefetch,
- const std::string& input_url = "https://www.google.com/search?q=foo") {
- AutocompleteInput input(
- // On IOS WEB/SRP, input text is not empty.
- is_ios ? base::ASCIIToUTF16(input_url) : u"",
- is_prefetch ? metrics::OmniboxEventProto::SRP_ZPS_PREFETCH
- : metrics::OmniboxEventProto::
- SEARCH_RESULT_PAGE_NO_SEARCH_TERM_REPLACEMENT,
- TestSchemeClassifier());
- input.set_current_url(GURL(input_url));
- input.set_focus_type(metrics::OmniboxFocusType::INTERACTION_FOCUS);
+ input.set_focus_type(user_input_in_progress
+ ? metrics::OmniboxFocusType::INTERACTION_DEFAULT
+ : metrics::OmniboxFocusType::INTERACTION_FOCUS);
return input;
}
@@ -213,6 +204,25 @@
return input;
}
+ // An AutocompleteInput that gets Zero Prefix Suggestions on SRP.
+ AutocompleteInput ZeroPrefixInputForSRP(
+ const bool is_prefetch,
+ const bool user_input_in_progress = false,
+ const std::string& input_url = "https://www.google.com/search?q=foo") {
+ AutocompleteInput input(
+ // On IOS WEB/SRP, input text is not empty.
+ is_ios ? base::ASCIIToUTF16(input_url) : u"",
+ is_prefetch ? metrics::OmniboxEventProto::SRP_ZPS_PREFETCH
+ : metrics::OmniboxEventProto::
+ SEARCH_RESULT_PAGE_NO_SEARCH_TERM_REPLACEMENT,
+ TestSchemeClassifier());
+ input.set_current_url(GURL(input_url));
+ input.set_focus_type(user_input_in_progress
+ ? metrics::OmniboxFocusType::INTERACTION_DEFAULT
+ : metrics::OmniboxFocusType::INTERACTION_FOCUS);
+ return input;
+ }
+
// An AutocompleteInput that gets Prefix Suggestions on SRP.
AutocompleteInput PrefixInputForSRP(
const std::string& input_url = "https://www.google.com/search?q=foo") {
@@ -363,17 +373,23 @@
// Tests whether zero-suggest is allowed on NTP/Web/SRP with various external
TEST_F(ZeroSuggestProviderTest, AllowZeroPrefixSuggestionsRequestEligibility) {
+ using ResultType = ZeroSuggestProvider::ResultType;
+
// Keep a reference to the Google default search provider.
TemplateURLService* template_url_service = client_->GetTemplateURLService();
const TemplateURL* google_provider =
template_url_service->GetDefaultSearchProvider();
// Benchmark test for NTP.
- auto test_ntp = [this]() {
- const auto& input = ZeroPrefixInputForNTP(/*is_prefetch=*/false);
+ auto test_ntp = [this](bool user_input_in_progress = false,
+ ResultType expected_result_type =
+ ResultType::kRemoteNoURL) {
+ const auto& input = ZeroPrefixInputForNTP(
+ /*is_prefetch=*/false,
+ /*user_input_in_progress=*/user_input_in_progress);
const auto [result_type, eligible] =
ZeroSuggestProvider::GetResultTypeAndEligibility(client_.get(), input);
- EXPECT_EQ(ZeroSuggestProvider::ResultType::kRemoteNoURL, result_type);
+ EXPECT_EQ(expected_result_type, result_type);
return eligible;
};
@@ -382,28 +398,36 @@
const auto& input = ZeroPrefixInputForLens();
const auto [result_type, eligible] =
ZeroSuggestProvider::GetResultTypeAndEligibility(client_.get(), input);
- EXPECT_EQ(ZeroSuggestProvider::ResultType::kRemoteNoURL, result_type);
+ EXPECT_EQ(ResultType::kRemoteNoURL, result_type);
return eligible;
};
// Benchmark test for valid page URL.
- auto test_other = [this]() {
- const auto& input = ZeroPrefixInputForWeb(/*is_prefetch=*/false);
+ auto test_other = [this](bool user_input_in_progress = false,
+ ResultType expected_result_type =
+ ResultType::kRemoteSendURL) {
+ const auto& input = ZeroPrefixInputForWeb(
+ /*is_prefetch=*/false,
+ /*user_input_in_progress=*/user_input_in_progress);
const auto [result_type, eligible] =
ZeroSuggestProvider::GetResultTypeAndEligibility(client_.get(), input);
- EXPECT_EQ(ZeroSuggestProvider::ResultType::kRemoteSendURL, result_type);
+ EXPECT_EQ(expected_result_type, result_type);
return eligible;
};
// Benchmark test for Search Results Page URL.
- auto test_srp = [this](const TemplateURL* template_url) {
+ auto test_srp = [this](const TemplateURL* template_url,
+ bool user_input_in_progress = false,
+ ResultType expected_result_type =
+ ResultType::kRemoteSendURL) {
const auto& input = ZeroPrefixInputForSRP(
/*is_prefetch=*/false,
+ /*user_input_in_progress=*/user_input_in_progress,
/*input_url= */
template_url->GenerateSearchURL(SearchTermsData()).spec());
const auto [result_type, eligible] =
ZeroSuggestProvider::GetResultTypeAndEligibility(client_.get(), input);
- EXPECT_EQ(ZeroSuggestProvider::ResultType::kRemoteSendURL, result_type);
+ EXPECT_EQ(expected_result_type, result_type);
return eligible;
};
@@ -414,26 +438,31 @@
// Zero-suggest is generally not allowed for invalid or non-HTTP(S) URLs.
AutocompleteInput on_focus_ineligible_url_input = ZeroPrefixInputForWeb(
/*is_prefetch=*/false,
+ /*user_input_in_progress=*/false,
/*input_url= */ "chrome://history");
- EXPECT_EQ(std::make_pair(ZeroSuggestProvider::ResultType::kNone, false),
+ EXPECT_EQ(std::make_pair(ResultType::kNone, false),
ZeroSuggestProvider::GetResultTypeAndEligibility(
client_.get(), on_focus_ineligible_url_input));
}
{
// Zero-suggest is generally not allowed for non-empty inputs.
- EXPECT_EQ(std::make_pair(ZeroSuggestProvider::ResultType::kNone, false),
+ EXPECT_EQ(std::make_pair(ResultType::kNone, false),
ZeroSuggestProvider::GetResultTypeAndEligibility(
client_.get(), PrefixInputForNTP()));
}
{
// Zero-suggest is generally not allowed for non-empty inputs.
- EXPECT_EQ(std::make_pair(ZeroSuggestProvider::ResultType::kNone, false),
+ EXPECT_EQ(std::make_pair(ResultType::kNone, false),
ZeroSuggestProvider::GetResultTypeAndEligibility(
client_.get(), PrefixInputForSRP()));
}
{
// Zero-suggest request can be made on NTP.
EXPECT_TRUE(test_ntp());
+
+ // Non-zero-prefix input should NOT result in zero-suggest request.
+ EXPECT_FALSE(test_ntp(/*user_input_in_progress=*/true,
+ /*expected_result_type=*/ResultType::kNone));
}
{
// Zero-suggest request can be made from Lens searchboxes.
@@ -443,10 +472,19 @@
{
// Valid SRP URLs can be sent in the zero-suggest request.
EXPECT_TRUE(test_srp(google_provider));
+
+ // Non-zero-prefix input should NOT result in a zero-suggest request.
+ EXPECT_FALSE(test_srp(google_provider,
+ /*user_input_in_progress=*/true,
+ /*expected_result_type=*/ResultType::kNone));
}
{
// Valid page URLs can be sent in the zero-suggest request.
EXPECT_TRUE(test_other());
+
+ // Non-zero-prefix input should NOT result in a zero-suggest request.
+ EXPECT_FALSE(test_other(/*user_input_in_progress=*/true,
+ /*expected_result_type=*/ResultType::kNone));
}
// Deactivate URL data collection. This ensures that the page URL
