[omnibox] ZeroSuggestProvider::TypeOfResultToRun w/ AutocompleteInput

This CL refactors ZeroSuggestProvider::TypeOfResultToRun to take an
AutocompleteInput as a parameter instead of just the current page URL.

We need this because differentiating the on-focus vs. on-clobber
ZeroSuggest modes requires access to AutocompleteInput::focus_type().

This CL also makes the method 'static', for three reasons:

 1. The function is more easily testable when it only depends on its
    parameters, rather than any state in the member variables.

 2. The member variables store data that's also in the |input|
    parameter, and I didn't want to introduce ambiguity as to which
    is used.

 3. We plan to split MostVisited into its own provider, and we will
    want to share some logic from TypeOfResultToRun. Making this
    method static is a step towards that future.

For reviewability, this CL actually has no functional changes, and
just moves things around.

The followup CL will actually add the on-clobber vs. on-focus logic to
TypeOfResultToRun.

Bug: 1106096
Change-Id: I69c8f2f08a182796be9ce1c66611fe6788a6158a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360562Reviewed-by: Tomasz Wiszkowski <ender@google.com>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799394}

components/omnibox/browser/zero_suggest_provider.cc

@@ -241,7 +241,7 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
   if (!suggest_url.is_valid())
     return;
 
-  result_type_running_ = TypeOfResultToRun(input.current_url(), suggest_url);
+  result_type_running_ = TypeOfResultToRun(client(), input, suggest_url);
   if (result_type_running_ == NONE)
     return;
 
@@ -674,26 +674,34 @@ void ZeroSuggestProvider::MaybeUseCachedSuggestions() {
   }
 }
 
+// static
 ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
-    const GURL& current_url,
+    AutocompleteProviderClient* client,
+    const AutocompleteInput& input,
     const GURL& suggest_url) {
+  DCHECK(client);
   // Check if the URL can be sent in any suggest request.
   const TemplateURLService* template_url_service =
-      client()->GetTemplateURLService();
+      client->GetTemplateURLService();
   DCHECK(template_url_service);
   const TemplateURL* default_provider =
       template_url_service->GetDefaultSearchProvider();
+
+  GURL current_url = input.current_url();
+  metrics::OmniboxEventProto::PageClassification current_page_classification =
+      input.current_page_classification();
+
   const bool can_send_current_url = CanSendURL(
-      current_url, suggest_url, default_provider, current_page_classification_,
-      template_url_service->search_terms_data(), client(), false);
+      current_url, suggest_url, default_provider, current_page_classification,
+      template_url_service->search_terms_data(), client, false);
   // Collect metrics on eligibility.
   GURL arbitrary_insecure_url(kArbitraryInsecureUrlString);
   ZeroSuggestEligibility eligibility = ZeroSuggestEligibility::ELIGIBLE;
   if (!can_send_current_url) {
     const bool can_send_ordinary_url =
         CanSendURL(arbitrary_insecure_url, suggest_url, default_provider,
-                   current_page_classification_,
-                   template_url_service->search_terms_data(), client(), false);
+                   current_page_classification,
+                   template_url_service->search_terms_data(), client, false);
     eligibility = can_send_ordinary_url
                       ? ZeroSuggestEligibility::URL_INELIGIBLE
                       : ZeroSuggestEligibility::GENERALLY_INELIGIBLE;
@@ -703,12 +711,12 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
       static_cast<int>(ZeroSuggestEligibility::ELIGIBLE_MAX_VALUE));
 
   const auto field_trial_variants =
-      OmniboxFieldTrial::GetZeroSuggestVariants(current_page_classification_);
+      OmniboxFieldTrial::GetZeroSuggestVariants(current_page_classification);
 
   if (base::Contains(field_trial_variants, kNoneVariant))
     return NONE;
 
-  if (current_page_classification_ == OmniboxEventProto::CHROMEOS_APP_LIST)
+  if (current_page_classification == OmniboxEventProto::CHROMEOS_APP_LIST)
     return REMOTE_NO_URL;
 
   // Contextual Open Web - (same client side behavior for multiple variants).
@@ -716,25 +724,25 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
       base::FeatureList::IsEnabled(omnibox::kOnFocusSuggestionsContextualWeb) ||
       base::FeatureList::IsEnabled(
           omnibox::kOnFocusSuggestionsContextualWebOnContent);
-  if (current_page_classification_ == OmniboxEventProto::OTHER &&
+  if (current_page_classification == OmniboxEventProto::OTHER &&
       can_send_current_url && contextual_web_suggestions_enabled) {
     return REMOTE_SEND_URL;
   }
 
   // Reactive Zero-Prefix Suggestions (rZPS) on NTP cases.
   bool remote_no_url_allowed =
-      RemoteNoUrlSuggestionsAreAllowed(client(), template_url_service);
+      RemoteNoUrlSuggestionsAreAllowed(client, template_url_service);
   if (remote_no_url_allowed) {
     // NTP Omnibox.
-    if ((current_page_classification_ == OmniboxEventProto::NTP ||
-         current_page_classification_ ==
+    if ((current_page_classification == OmniboxEventProto::NTP ||
+         current_page_classification ==
              OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS) &&
         base::FeatureList::IsEnabled(
             omnibox::kReactiveZeroSuggestionsOnNTPOmnibox)) {
       return REMOTE_NO_URL;
     }
     // NTP Realbox.
-    if (current_page_classification_ == OmniboxEventProto::NTP_REALBOX &&
+    if (current_page_classification == OmniboxEventProto::NTP_REALBOX &&
         base::FeatureList::IsEnabled(
             omnibox::kReactiveZeroSuggestionsOnNTPRealbox)) {
       return REMOTE_NO_URL;
@@ -755,13 +763,13 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
 
 #if !defined(OS_IOS)
   // For Desktop and Android, default to REMOTE_NO_URL on the NTP, if allowed.
-  if (IsNTPPage(current_page_classification_) && remote_no_url_allowed)
+  if (IsNTPPage(current_page_classification) && remote_no_url_allowed)
     return REMOTE_NO_URL;
 #endif
 
 #if defined(OS_ANDROID) || defined(OS_IOS)
   // For Android and iOS, default to MOST_VISITED everywhere except on the SERP.
-  if (!IsSearchResultsPage(current_page_classification_)) {
+  if (!IsSearchResultsPage(current_page_classification)) {
     return MOST_VISITED;
   }
 #endif

components/omnibox/browser/zero_suggest_provider.h

@@ -66,13 +66,6 @@ class ZeroSuggestProvider : public BaseSearchProvider {
   // Sets |field_trial_triggered_| to false.
   void ResetSession() override;
 
-  // Calling |Start()| will reset the page classification. This is mainly
-  // intended for unit testing TypeOfResultToRun().
-  void SetPageClassificationForTesting(
-      metrics::OmniboxEventProto::PageClassification classification) {
-    current_page_classification_ = classification;
-  }
-
   // Returns the list of experiment stats corresponding to the latest |results_|
   // to be logged to SearchboxStats as part of a GWS experiment, if any.
   const SearchSuggestionParser::ExperimentStats& experiment_stats() const {
@@ -187,8 +180,12 @@ class ZeroSuggestProvider : public BaseSearchProvider {
   // context.
   // Logs UMA metrics. Should be called exactly once, on Start(), otherwise the
   // meaning of the data logged would change.
-  ResultType TypeOfResultToRun(const GURL& current_url,
-                               const GURL& suggest_url);
+  //
+  // This method is static for testability and to avoid depending on the
+  // provider state.
+  static ResultType TypeOfResultToRun(AutocompleteProviderClient* client,
+                                      const AutocompleteInput& input,
+                                      const GURL& suggest_url);
 
   AutocompleteProviderListener* listener_;
 

components/omnibox/browser/zero_suggest_provider_unittest.cc

@@ -210,7 +210,6 @@ void ZeroSuggestProviderTest::SetZeroSuggestVariantForAllContexts(
 }
 
 TEST_F(ZeroSuggestProviderTest, AllowZeroSuggestSuggestions) {
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::OTHER);
   std::string input_url = "https://example.com/";
 
   AutocompleteInput prefix_input(base::ASCIIToUTF16(input_url),
@@ -262,17 +261,17 @@ TEST_F(ZeroSuggestProviderTest, AllowZeroSuggestSuggestions) {
 }
 
 TEST_F(ZeroSuggestProviderTest, TypeOfResultToRun) {
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::OTHER);
-  GURL current_url = GURL("https://example.com/");
-  GURL suggest_url = GetSuggestURL(metrics::OmniboxEventProto::OTHER);
-
   // Verifies the unconfigured state. Returns platorm-specific defaults.
+  // TODO(tommycli): The remote_no_url_allowed idiom seems kind of confusing,
+  // its true meaning seems closer to "expect_remote_no_url". Ideally we can
+  // simplify the test or make this much more obvious.
   auto ExpectPlatformSpecificDefaultZeroSuggestBehavior =
-      [&](const bool remote_no_url_allowed) {
+      [&](AutocompleteInput& input, const bool remote_no_url_allowed) {
         const auto current_page_classification =
-            provider_->current_page_classification_;
-        const auto result_type =
-            provider_->TypeOfResultToRun(current_url, suggest_url);
+            input.current_page_classification();
+        GURL suggest_url = GetSuggestURL(current_page_classification);
+        const auto result_type = ZeroSuggestProvider::TypeOfResultToRun(
+            client_.get(), input, suggest_url);
 #if !defined(OS_ANDROID) && !defined(OS_IOS)  // Desktop
         EXPECT_EQ(BaseSearchProvider::IsNTPPage(current_page_classification) &&
                           remote_no_url_allowed
@@ -296,37 +295,42 @@ TEST_F(ZeroSuggestProviderTest, TypeOfResultToRun) {
                   result_type);
 #endif
       };
+
+  // Verify OTHER defaults (contextual web).
+  std::string url("https://www.example.com/");
+  AutocompleteInput other_input(base::ASCIIToUTF16(url),
+                                metrics::OmniboxEventProto::OTHER,
+                                TestSchemeClassifier());
+  other_input.set_current_url(GURL(url));
   ExpectPlatformSpecificDefaultZeroSuggestBehavior(
+      other_input,
       /*remote_no_url_allowed=*/false);
 
   // Verify the platorm-specific defaults for the NTP.
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::NTP);
-  suggest_url = GetSuggestURL(metrics::OmniboxEventProto::NTP);
+  AutocompleteInput ntp_input(base::string16(), metrics::OmniboxEventProto::NTP,
+                              TestSchemeClassifier());
   ExpectPlatformSpecificDefaultZeroSuggestBehavior(
+      ntp_input,
       /*remote_no_url_allowed=*/false);
 
   // Verify RemoteNoUrl works when the user is signed in.
   EXPECT_CALL(*client_, IsAuthenticated())
       .WillRepeatedly(testing::Return(true));
-
-  // Verify the platorm-specific defaults for the NTP.
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::NTP);
-  suggest_url = GetSuggestURL(metrics::OmniboxEventProto::NTP);
   ExpectPlatformSpecificDefaultZeroSuggestBehavior(
+      ntp_input,
       /*remote_no_url_allowed=*/true);
 
-  // Restore to non-NTP page classification.
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::OTHER);
-  suggest_url = GetSuggestURL(metrics::OmniboxEventProto::OTHER);
-
   CreateRemoteNoUrlFieldTrial();
+  GURL other_suggest_url = GetSuggestURL(metrics::OmniboxEventProto::OTHER);
   EXPECT_EQ(ZeroSuggestProvider::ResultType::REMOTE_NO_URL,
-            provider_->TypeOfResultToRun(current_url, suggest_url));
+            ZeroSuggestProvider::TypeOfResultToRun(client_.get(), other_input,
+                                                   other_suggest_url));
 
   // But if the user has signed out, fall back to platform-specific defaults.
   EXPECT_CALL(*client_, IsAuthenticated())
       .WillRepeatedly(testing::Return(false));
   ExpectPlatformSpecificDefaultZeroSuggestBehavior(
+      other_input,
       /*remote_no_url_allowed=*/false);
 
   // Restore authentication state, but now set a non-Google default search
@@ -342,6 +346,7 @@ TEST_F(ZeroSuggestProviderTest, TypeOfResultToRun) {
       turl_model->Add(std::make_unique<TemplateURL>(data));
   turl_model->SetUserSelectedDefaultSearchProvider(other_search_provider);
   ExpectPlatformSpecificDefaultZeroSuggestBehavior(
+      other_input,
       /*remote_no_url_allowed=*/false);
 
   // Restore Google as the default search provider.
@@ -352,13 +357,16 @@ TEST_F(ZeroSuggestProviderTest, TypeOfResultToRun) {
   SetZeroSuggestVariantForAllContexts(
       ZeroSuggestProvider::kRemoteSendUrlVariant);
   EXPECT_EQ(ZeroSuggestProvider::ResultType::REMOTE_SEND_URL,
-            provider_->TypeOfResultToRun(current_url, suggest_url));
+            ZeroSuggestProvider::TypeOfResultToRun(client_.get(), other_input,
+                                                   other_suggest_url));
   CreateRemoteNoUrlFieldTrial();
   EXPECT_EQ(ZeroSuggestProvider::ResultType::REMOTE_NO_URL,
-            provider_->TypeOfResultToRun(current_url, suggest_url));
+            ZeroSuggestProvider::TypeOfResultToRun(client_.get(), other_input,
+                                                   other_suggest_url));
   CreateMostVisitedFieldTrial();
   EXPECT_EQ(ZeroSuggestProvider::ResultType::MOST_VISITED,
-            provider_->TypeOfResultToRun(current_url, suggest_url));
+            ZeroSuggestProvider::TypeOfResultToRun(client_.get(), other_input,
+                                                   other_suggest_url));
 
   // Verify that a wildcard rule works in conjunction with a
   // page-classification-specific rule.
@@ -373,13 +381,22 @@ TEST_F(ZeroSuggestProviderTest, TypeOfResultToRun) {
                               metrics::OmniboxEventProto::BLANK),
            ZeroSuggestProvider::kNoneVariant},
       });
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::OTHER);
   EXPECT_EQ(ZeroSuggestProvider::ResultType::MOST_VISITED,
-            provider_->TypeOfResultToRun(current_url, suggest_url));
-  provider_->SetPageClassificationForTesting(metrics::OmniboxEventProto::BLANK);
-  EXPECT_EQ(
-      ZeroSuggestProvider::ResultType::NONE,
-      provider_->TypeOfResultToRun(GURL("chrome://newtab/"), suggest_url));
+            ZeroSuggestProvider::TypeOfResultToRun(client_.get(), other_input,
+                                                   other_suggest_url));
+
+  // Test the BLANK page classification to verify the wildcard rule works.
+  {
+    std::string url("chrome://newtab/");
+    AutocompleteInput blank_input(base::ASCIIToUTF16(url),
+                                  metrics::OmniboxEventProto::BLANK,
+                                  TestSchemeClassifier());
+    blank_input.set_current_url(GURL(url));
+    EXPECT_EQ(ZeroSuggestProvider::ResultType::NONE,
+              ZeroSuggestProvider::TypeOfResultToRun(
+                  client_.get(), blank_input,
+                  GetSuggestURL(metrics::OmniboxEventProto::BLANK)));
+  }
 }
 
 TEST_F(ZeroSuggestProviderTest, TestDoesNotReturnMatchesForPrefix) {