Do not delay most visited results.

chrome/browser/autocomplete/zero_suggest_provider.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/zero_suggest_provider.h"
 
 #include "base/callback.h"
 #include "base/i18n/case_conversion.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/user_metrics.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/metrics/proto/omnibox_input_type.pb.h"
 #include "components/omnibox/autocomplete_input.h"
 #include "components/omnibox/autocomplete_match.h"
 #include "components/omnibox/autocomplete_provider_listener.h"
 #include "components/omnibox/omnibox_field_trial.h"
 #include "components/omnibox/search_provider.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/search_engines/template_url_service.h"
@@ skipping 75 matching lines @@
   if (default_provider == NULL)
     return;
 
   base::string16 prefix;
   TemplateURLRef::SearchTermsArgs search_term_args(prefix);
   GURL suggest_url(default_provider->suggestions_url_ref().ReplaceSearchTerms(
       search_term_args, template_url_service_->search_terms_data()));
   if (!suggest_url.is_valid())
     return;
 
-  // No need to send the current page URL in personalized suggest field trial.
+  // No need to send the current page URL in personalized suggest or
+  // most visited field trials.
   if (CanSendURL(input.current_url(), suggest_url, default_provider,
                  current_page_classification_,
                  template_url_service_->search_terms_data(), client_.get()) &&
-      !OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial()) {
+      !OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial() &&
+      !OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
     // Update suggest_url to include the current_page_url.
     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 (!CanShowZeroSuggestWithoutSendingURL(suggest_url,
-                                                  input.current_url())) {
+  } else if (!ShouldShowNonContextualZeroSuggest(suggest_url,
+                                                 input.current_url())) {
     return;
   }
 
   done_ = false;
   // 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);
 }
 
 void ZeroSuggestProvider::Stop(bool clear_cached_results) {
   if (fetcher_)
     LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_INVALIDATED);
   fetcher_.reset();
   done_ = true;
 

[Mark P, 2014/10/21 19:08:06]

Forgive me that I don't fully understand the weak pointer Bind() callbacks, but
shouldn't we need to cancel the GetMostVisitedURLs() call here?  Otherwise we
may end up sending such a call, then the user navigates to a SRP page, the user
clicks in the omnibox again, then the call returns, and we displaying the Most
Visited URLs even though we're not supposed to.  (We only check whether they
should be displayed on a page when we send the call, and I don't see any
canceling the call.)

[Maria, 2014/10/21 23:57:39]

You are right. Added code to cover this case and also added a unit test to
verify it works.

   if (clear_cached_results) {
     // We do not call Clear() on |results_| to retain |verbatim_relevance|
     // value in the |results_| object. |verbatim_relevance| is used at the
     // beginning of the next StartZeroSuggest() call to determine the current
     // url match relevance.
     results_.suggest_results.clear();
     results_.navigation_results.clear();
     current_query_.clear();
   }
 }
 
 void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) {

[Mark P, 2014/10/21 19:08:06]

Can we do something here in the Most Visited case?

[Maria, 2014/10/21 23:57:39]

Currently the matches are not marked as deletable. If we were to mark them as
deletable, then the question would be what are the semantics of deleting them.
The thing that would make sense is removing the URL from most visited page (and
from history?). On the other hand it's a bit destructive and would probably
require us changing the UI for most visited case specifically to explain what we
are going to do. Not entirely sure whether this is worth doing, considering that
we will probably be switching these suggestions for most likely before too long.

Let me know what you think.

[Mark P, 2014/10/22 19:11:23]

That sounds fine.  I just wanted to make sure you thought about this.

   if (OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial()) {
     // Remove the deleted match from the cache, so it is not shown to the user
     // again. Since we cannot remove just one result, blow away the cache.
     profile_->GetPrefs()->SetString(prefs::kZeroSuggestCachedResults,
                                     std::string());
   }
   BaseSearchProvider::DeleteMatch(match);
 }
 
 void ZeroSuggestProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
@@ skipping 131 matching lines @@
 
   match.description =
       AutocompleteMatch::SanitizeString(navigation.description());
   AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
       match.description.length(), ACMatchClassification::NONE,
       &match.description_class);
   return match;
 }
 
 void ZeroSuggestProvider::Run(const GURL& suggest_url) {
-  const int kFetcherID = 1;
-  fetcher_.reset(
-      net::URLFetcher::Create(kFetcherID,
-          suggest_url,
-          net::URLFetcher::GET, this));
-  fetcher_->SetRequestContext(profile_->GetRequestContext());
-  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
-  // Add Chrome experiment state to the request headers.
-  net::HttpRequestHeaders headers;
-  variations::VariationsHttpHeaderProvider::GetInstance()->AppendHeaders(
-      fetcher_->GetOriginalURL(), profile_->IsOffTheRecord(), false, &headers);
-  fetcher_->SetExtraRequestHeaders(headers.ToString());
-  fetcher_->Start();
-
   if (OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial()) {
     most_visited_urls_.clear();
     history::TopSites* ts = profile_->GetTopSites();
     if (ts) {
       ts->GetMostVisitedURLs(
           base::Bind(&ZeroSuggestProvider::OnMostVisitedUrlsAvailable,
                      weak_ptr_factory_.GetWeakPtr()), false);
     }
+  } else {
+    const int kFetcherID = 1;
+    fetcher_.reset(
+        net::URLFetcher::Create(kFetcherID,
+            suggest_url,
+            net::URLFetcher::GET, this));
+    fetcher_->SetRequestContext(profile_->GetRequestContext());
+    fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
+    // Add Chrome experiment state to the request headers.
+    net::HttpRequestHeaders headers;
+    variations::VariationsHttpHeaderProvider::GetInstance()->AppendHeaders(
+        fetcher_->GetOriginalURL(), profile_->IsOffTheRecord(), false,
+        &headers);
+    fetcher_->SetExtraRequestHeaders(headers.ToString());
+    fetcher_->Start();
+    LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);
   }
-  LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);
 }
 
 void ZeroSuggestProvider::OnMostVisitedUrlsAvailable(
     const history::MostVisitedURLList& urls) {
   most_visited_urls_ = urls;
+  done_ = true;
+  ConvertResultsToAutocompleteMatches();
+  listener_->OnProviderUpdate(true);
 }
 
 void ZeroSuggestProvider::ConvertResultsToAutocompleteMatches() {
   matches_.clear();
 
   const TemplateURL* default_provider =
       template_url_service_->GetDefaultSearchProvider();
   // Fail if we can't set the clickthrough URL for query suggestions.
   if (default_provider == NULL || !default_provider->SupportsReplacement(
           template_url_service_->search_terms_data()))
@@ skipping 66 matching lines @@
   match.relevance = GetVerbatimRelevance();
 
   return match;
 }
 
 int ZeroSuggestProvider::GetVerbatimRelevance() const {
   return results_.verbatim_relevance >= 0 ?
       results_.verbatim_relevance : kDefaultVerbatimZeroSuggestRelevance;
 }
 
-bool ZeroSuggestProvider::CanShowZeroSuggestWithoutSendingURL(
+bool ZeroSuggestProvider::ShouldShowNonContextualZeroSuggest(
     const GURL& suggest_url,
     const GURL& current_page_url) const {
   if (!ZeroSuggestEnabled(suggest_url,
                           template_url_service_->GetDefaultSearchProvider(),
                           current_page_classification_,
                           template_url_service_->search_terms_data(),
                           client_.get()))
     return false;
 
   // If we cannot send URLs, then only the MostVisited and Personalized
   // variations can be shown.
   if (!OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial() &&
       !OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial())
     return false;
 
   // Only show zero suggest for HTTP[S] pages.
   // TODO(mariakhomenko): We may be able to expand this set to include pages
   // with other schemes (e.g. chrome://). That may require improvements to
   // the formatting of the verbatim result returned by MatchForCurrentURL().
   if (!current_page_url.is_valid() ||
       ((current_page_url.scheme() != url::kHttpScheme) &&
       (current_page_url.scheme() != url::kHttpsScheme)))
     return false;
 
+  if (OmniboxFieldTrial::InZeroSuggestMostVisitedWithoutSerpFieldTrial() &&
+      TemplateURLServiceFactory::GetForProfile(profile_)->
+          IsSearchResultsPageFromDefaultSearchProvider(current_page_url))

[Mark P, 2014/10/21 19:08:06]

Generally I understand this works (at least on Android).  However, isn't there's
a better way to check "is doing search term replacement"?  I thought that's what
we agreed to.  Checking that we're on a search results page isn't equivalent.

[Maria, 2014/10/21 23:57:39]

Actually, I thought this is exactly what we discussed. Basically if we are on a
search results page, we don't want to "distract" users from searching.

Currently there isn't a way to tell on Android whether we are doing search term
replacement (but I believe we always do it on a search page). We could
eventually check current_page_classification_ for search term replacement case,
but currently it isn't being set correctly on Android. (I have a task to fix
that)

[Mark P, 2014/10/22 19:11:23]

Okay, good to know.  Is there a bug I can follow about the wrong page
classification on Android?  I wasn't aware of this.

Do we want a TODO or a crbug or action item to follow up on whether this should
be based on search term replacement.  After all, I can easily imagine turning it
on other platforms, some of which do this and some do not (and sometimes it
depends on various factors)...

[Maria, 2014/10/23 20:34:26]

Tracked in https://code.google.com/p/chromium/issues/detail?id=420717

+    return false;
+
   return true;
 }
 
 void ZeroSuggestProvider::MaybeUseCachedSuggestions() {
   if (!OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial())
     return;
 
   std::string json_data = profile_->GetPrefs()->GetString(
       prefs::kZeroSuggestCachedResults);
   if (!json_data.empty()) {
     scoped_ptr<base::Value> data(
         SearchSuggestionParser::DeserializeJsonData(json_data));
     if (data && ParseSuggestResults(
             *data, kDefaultZeroSuggestRelevance, false, &results_)) {
       ConvertResultsToAutocompleteMatches();
       results_from_cache_ = !matches_.empty();
     }
   }
 }