Add ContextualSuggestionsService to Omnibox

components/omnibox/browser/zero_suggest_provider.cc

Index: components/omnibox/browser/zero_suggest_provider.cc
diff --git a/components/omnibox/browser/zero_suggest_provider.cc b/components/omnibox/browser/zero_suggest_provider.cc
index 45e7d86b1172c646767a6874edd5866065ef9424..c1da9452b4c030182477cb2b136e87b949c979cf 100644
--- a/components/omnibox/browser/zero_suggest_provider.cc
+++ b/components/omnibox/browser/zero_suggest_provider.cc
@@ -26,6 +26,7 @@
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_provider_listener.h"
+#include "components/omnibox/browser/contextual_suggestions_service.h"
 #include "components/omnibox/browser/history_url_provider.h"
 #include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/omnibox_pref_names.h"
@@ -85,32 +86,6 @@ const int kDefaultZeroSuggestRelevance = 100;
 // Used for testing whether zero suggest is ever available.
 constexpr char kArbitraryInsecureUrlString[] = "http://www.google.com/";
 
-// Returns true if the folowing conditions are valid:
-// * The |default_provider| is Google.
-// * The user is in the ZeroSuggestRedirectToChrome field trial.
-// * The field trial provides a valid server address where the suggest request
-//   is redirected.
-// * The suggest request is over HTTPS. This avoids leaking the current page URL
-//   or personal data in unencrypted network traffic.
-// Note: these checks are in addition to CanSendUrl() on the default contextual
-// suggestion URL.
-bool UseExperimentalSuggestService(const TemplateURLService& default_provider) {
-  const TemplateURL& default_provider_url =
-      *default_provider.GetDefaultSearchProvider();
-  const SearchTermsData& search_terms_data =
-      default_provider.search_terms_data();
-  if ((default_provider_url.GetEngineType(search_terms_data) !=
-       SEARCH_ENGINE_GOOGLE) ||
-      !OmniboxFieldTrial::InZeroSuggestRedirectToChromeFieldTrial())
-    return false;
-  // Check that the suggest URL for redirect to chrome field trial is valid.
-  const GURL suggest_url(
-      OmniboxFieldTrial::ZeroSuggestRedirectToChromeServerAddress());
-  if (!suggest_url.is_valid())
-    return false;
-  return suggest_url.SchemeIsCryptographic();
-}
-
 }  // namespace
 
 // static
@@ -174,33 +149,56 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
   UMA_HISTOGRAM_ENUMERATION(
       "Omnibox.ZeroSuggest.Eligible.OnFocus", static_cast<int>(eligibility),
       static_cast<int>(ZeroSuggestEligibility::ELIGIBLE_MAX_VALUE));
-  if (can_send_current_url &&
+
+  bool attach_current_url =

[Mark P, 2017/07/18 19:11:30]

optional nit: how about can_attach_current_url?

+      can_send_current_url &&
       !OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial() &&
-      !OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
-    // Update suggest_url to include the current_page_url.
-    if (UseExperimentalSuggestService(*template_url_service)) {
-      suggest_url = GURL(
-          OmniboxFieldTrial::ZeroSuggestRedirectToChromeServerAddress() +
-          "/url=" + net::EscapePath(current_query_) +
-          OmniboxFieldTrial::ZeroSuggestRedirectToChromeAdditionalFields());
-    } else {
-      base::string16 prefix;
-      TemplateURLRef::SearchTermsArgs search_term_args(prefix);
-      search_term_args.current_page_url = current_query_;
-      suggest_url =
-          GURL(default_provider->suggestions_url_ref().ReplaceSearchTerms(
-              search_term_args, template_url_service->search_terms_data()));
-    }
-  } else if (!ShouldShowNonContextualZeroSuggest(input.current_url())) {
+      !OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial();
+
+  if (!attach_current_url &&
+      !ShouldShowNonContextualZeroSuggest(input.current_url())) {
     return;
   }
 
-  done_ = false;

[Mark P, 2017/07/18 19:11:30]

I think this line should probably stay.  I wonder why Gheorghe removed it.

   // TODO(jered): Consider adding locally-sourced zero-suggestions here too.
   // These may be useful on the NTP or more relevant to the user than server
   // suggestions, if based on local browsing history.
   MaybeUseCachedSuggestions();
-  Run(suggest_url);
+
+  if (OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
+    most_visited_urls_.clear();
+    scoped_refptr<history::TopSites> ts = client()->GetTopSites();
+    if (ts) {
+      waiting_for_most_visited_urls_request_ = true;
+      ts->GetMostVisitedURLs(
+          base::Bind(&ZeroSuggestProvider::OnMostVisitedUrlsAvailable,
+                     weak_ptr_factory_.GetWeakPtr()),
+          false);
+    }
+    return;
+  }
+
+  // Create a request for experimental suggestions with |this| as the
+  // fetcher delegate.
+  client()
+      ->GetContextualSuggestionsService()

[Mark P, 2017/07/18 19:11:30]

Is GetContextualSuggestionsService() always going to be non-null?

+      ->CreateContextualSuggestionsRequest(
+          attach_current_url ? current_query_ : std::string(),
+          /*fetcher_delegate=*/this,
+          base::BindOnce(
+              &ZeroSuggestProvider::OnContextualSuggestionsFetcherAvailable,
+              weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ZeroSuggestProvider::OnContextualSuggestionsFetcherAvailable(

[Mark P, 2017/07/18 19:11:30]

nit: Definitions in the .cc file should be in the same order at in the header.

+    std::unique_ptr<net::URLFetcher> fetcher) {
+  if (fetcher == nullptr) {

[Mark P, 2017/07/18 19:11:30]

Why / how can this happen?
Also, since we're going to get any additional suggestions after this failure, I
think you should set done_ to true here.

+    return;
+  }
+  fetcher_ = std::move(fetcher);
+  done_ = false;

[Mark P, 2017/07/18 19:11:30]

I don't think this is the right place to set done_ to false.  I think it should
remain set to false where it was.

+  fetcher_->Start();
+  LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);
 }
 
 void ZeroSuggestProvider::Stop(bool clear_cached_results,
@@ -396,68 +394,6 @@ AutocompleteMatch ZeroSuggestProvider::NavigationToMatch(
   return match;
 }
 
-void ZeroSuggestProvider::Run(const GURL& suggest_url) {
-  if (OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
-    most_visited_urls_.clear();
-    scoped_refptr<history::TopSites> ts = client()->GetTopSites();
-    if (ts) {
-      waiting_for_most_visited_urls_request_ = true;
-      ts->GetMostVisitedURLs(
-          base::Bind(&ZeroSuggestProvider::OnMostVisitedUrlsAvailable,
-                     weak_ptr_factory_.GetWeakPtr()), false);
-    }
-  } else {
-    net::NetworkTrafficAnnotationTag traffic_annotation =
-        net::DefineNetworkTrafficAnnotation("omnibox_zerosuggest", R"(
-        semantics {
-          sender: "Omnibox"
-          description:
-            "When the user focuses the omnibox, Chrome can provide search or "
-            "navigation suggestions from the default search provider in the "
-            "omnibox dropdown, based on the current page URL.\n"
-            "This is limited to users whose default search engine is Google, "
-            "as no other search engines currently support this kind of "
-            "suggestion."
-          trigger: "The omnibox receives focus."
-          data: "The URL of the current page."
-          destination: GOOGLE_OWNED_SERVICE
-        }
-        policy {
-          cookies_allowed: true
-          cookies_store: "user"
-          setting:
-            "Users can control this feature via the 'Use a prediction service "
-            "to help complete searches and URLs typed in the address bar' "
-            "settings under 'Privacy'. The feature is enabled by default."
-          chrome_policy {
-            SearchSuggestEnabled {
-                policy_options {mode: MANDATORY}
-                SearchSuggestEnabled: false
-            }
-          }
-        })");
-    const int kFetcherID = 1;
-    fetcher_ =
-        net::URLFetcher::Create(kFetcherID, suggest_url, net::URLFetcher::GET,
-                                this, traffic_annotation);
-    data_use_measurement::DataUseUserData::AttachToFetcher(
-        fetcher_.get(), data_use_measurement::DataUseUserData::OMNIBOX);
-    fetcher_->SetRequestContext(client()->GetRequestContext());
-    fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
-    // Add Chrome experiment state to the request headers.
-    net::HttpRequestHeaders headers;
-    // Note: It's OK to pass |is_signed_in| false if it's unknown, as it does
-    // not affect transmission of experiments coming from the variations server.
-    bool is_signed_in = false;
-    variations::AppendVariationHeaders(fetcher_->GetOriginalURL(),
-                                       client()->IsOffTheRecord(), false,
-                                       is_signed_in, &headers);
-    fetcher_->SetExtraRequestHeaders(headers.ToString());
-    fetcher_->Start();
-    LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);
-  }
-}
-
 void ZeroSuggestProvider::OnMostVisitedUrlsAvailable(
     const history::MostVisitedURLList& urls) {
   if (!waiting_for_most_visited_urls_request_) return;