Part 4 of search provider refactoring.

chrome/browser/autocomplete/base_search_provider.cc

Index: chrome/browser/autocomplete/base_search_provider.cc
diff --git a/chrome/browser/autocomplete/base_search_provider.cc b/chrome/browser/autocomplete/base_search_provider.cc
index bb60179625cf07bff618a609b9f925700fabad17..a539c3a407867eab9a1ac4d959263b7942639b92 100644
--- a/chrome/browser/autocomplete/base_search_provider.cc
+++ b/chrome/browser/autocomplete/base_search_provider.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/autocomplete/base_search_provider.h"
 
+#include "base/i18n/case_conversion.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ -12,6 +13,9 @@
 #include "chrome/browser/autocomplete/url_prefix.h"
 #include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/instant_service.h"
+#include "chrome/browser/search/instant_service_factory.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -33,6 +37,11 @@ BaseSearchProvider::BaseSearchProvider(AutocompleteProviderListener* listener,
       field_trial_triggered_(false),
       field_trial_triggered_in_session_(false) {}
 
+// static
+bool BaseSearchProvider::ShouldPrefetch(const AutocompleteMatch& match) {
+  return match.GetAdditionalInfo(kShouldPrefetchKey) == kTrue;
+}
+
 void BaseSearchProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
   provider_info->push_back(metrics::OmniboxEventProto_ProviderInfo());
   metrics::OmniboxEventProto_ProviderInfo& new_entry = provider_info->back();
@@ -254,6 +263,14 @@ bool BaseSearchProvider::Results::HasServerProvidedScores() const {
 
 // BaseSearchProvider ---------------------------------------------------------
 
+const char BaseSearchProvider::kRelevanceFromServerKey[] =

[Mark P, 2014/02/10 23:25:55]

nit: comment // static

[Maria, 2014/02/11 21:42:32]

Done.

+    "relevance_from_server";
+const char BaseSearchProvider::kShouldPrefetchKey[] = "should_prefetch";
+const char BaseSearchProvider::kSuggestMetadataKey[] = "suggest_metadata";
+const char BaseSearchProvider::kDeletionUrlKey[] = "deletion_url";
+const char BaseSearchProvider::kTrue[] = "true";
+const char BaseSearchProvider::kFalse[] = "false";
+
 // static
 AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     AutocompleteProvider* autocomplete_provider,
@@ -403,3 +420,83 @@ bool BaseSearchProvider::CanSendURL(
   return true;
 }
 
+void BaseSearchProvider::AddMatchToMap(const SuggestResult& result,
+                                       const AutocompleteInput input,
+                                       const base::string16& query_string,
+                                       const TemplateURL* template_url,
+                                       const std::string& metadata,
+                                       int accepted_suggestion,
+                                       bool append_extra_query_params,
+                                       MatchMap* map) {
+  InstantService* instant_service =
+      InstantServiceFactory::GetForProfile(profile_);
+  // Android and iOS have on InstantService
+  const int omnibox_start_margin = instant_service ?
+      instant_service->omnibox_start_margin() : chrome::kDisableStartMargin;
+
+  AutocompleteMatch match = CreateSearchSuggestion(this, input, query_string,

[Mark P, 2014/02/10 23:25:55]

nit: formatting.  (I think it's either always-align or always-indent-four, not a
mix of the two.)

[Mark P, 2014/02/10 23:25:55]

You've switched this from |input_| to |input_| or |keyword_input_|, depending on
which |result| is from.  In SearchProvider, we unconditionally used |input_|
here.  I audited the code and think this either-or will be okay.

This raises a side comment: you might as well remove the function you added
GetInput() because there's no need to select input types based on result.

Or you can pass the correct input (default or keyword as appropriate) and skip
passing input_text, because this is always the correct input.text()

[Maria, 2014/02/11 01:22:09]

Thanks for catching this. The switch to matching input object was unintentional.

In terms of search_provider, I agree that we can do either: pass the correct
input OR always pass input_ along with the query (which is today's
functionality).

In terms of zero_suggest_provider however, we seem to do something a bit weird.
We construct an empty AutocompleteInput object and then pass the suggestion text
as the query. So even though the real query in zero_suggest_provider is always
"", we send the suggestion text as an input (as far as I can tell).

One slightly cleaner way may be to pass the exact parts of the input that we
need into CreateSearchSuggestion (input_text, input_type,
prevent_inline_autocomplete) and have each provider pass correct values for
those from AddMatchToMap.

Another way would be to only pass AutocompleteInput object, but mangle
zero_suggest_provider's AutocompleteInput to actually set text to the value we
are interested in retrieving -- this will be a bit weird, but will require
passing fewer parameters.

Opinions on which one makes more sense?

[Maria, 2014/02/11 21:42:32]

Done.

+      result, template_url, accepted_suggestion, omnibox_start_margin,
+      append_extra_query_params);
+  if (!match.destination_url.is_valid())
+    return;
+  match.search_terms_args->bookmark_bar_pinned =
+      profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
+  match.RecordAdditionalInfo(kRelevanceFromServerKey,
+                             result.relevance_from_server() ? kTrue : kFalse);
+  match.RecordAdditionalInfo(kShouldPrefetchKey,
+                             result.should_prefetch() ? kTrue : kFalse);
+
+  if (!result.deletion_url().empty()) {
+    GURL url(match.destination_url.GetOrigin().Resolve(result.deletion_url()));
+    if (url.is_valid()) {
+      match.RecordAdditionalInfo(kDeletionUrlKey, url.spec());
+      match.deletable = true;
+    }
+  }
+
+  // Metadata is needed only for prefetching queries.
+  if (result.should_prefetch())
+    match.RecordAdditionalInfo(kSuggestMetadataKey, metadata);
+
+  // Try to add |match| to |map|.  If a match for |query_string| is already in
+  // |map|, replace it if |match| is more relevant.
+  // NOTE: Keep this ToLower() call in sync with url_database.cc.
+  MatchKey match_key(
+      std::make_pair(base::i18n::ToLower(result.suggestion()),
+                     match.search_terms_args->suggest_query_params));
+  const std::pair<MatchMap::iterator, bool> i(
+       map->insert(std::make_pair(match_key, match)));
+
+  bool should_prefetch = result.should_prefetch();
+  if (!i.second) {
+     // NOTE: We purposefully do a direct relevance comparison here instead of
+     // using AutocompleteMatch::MoreRelevant(), so that we'll prefer "items
+     // added first" rather than "items alphabetically first" when the scores
+     // are equal. The only case this matters is when a user has results with
+     // the same score that differ only by capitalization; because the history
+     // system returns results sorted by recency, this means we'll pick the most
+     // recent such result even if the precision of our relevance score is too
+     // low to distinguish the two.
+     if (match.relevance > i.first->second.relevance) {
+       i.first->second = match;
+     } else if (match.keyword == i.first->second.keyword) {
+       // Old and new matches are from the same search provider. It is okay to
+       // record one match's prefetch data onto a different match (for the same
+       // query string) for the following reasons:
+       // 1. Because the suggest server only sends down a query string from
+       // which we construct a URL, rather than sending a full URL, and because
+       // we construct URLs from query strings in the same way every time, the
+       // URLs for the two matches will be the same. Therefore, we won't end up
+       // prefetching something the server didn't intend.
+       // 2. Presumably the server sets the prefetch bit on a match it things is
+       // sufficiently relevant that the user is likely to choose it. Surely
+       // setting the prefetch bit on a match of even higher relevance won't
+       // violate this assumption.
+       should_prefetch |= ShouldPrefetch(i.first->second);
+       i.first->second.RecordAdditionalInfo(kShouldPrefetchKey,
+                                            should_prefetch ? kTrue : kFalse);
+       if (should_prefetch)
+         i.first->second.RecordAdditionalInfo(kSuggestMetadataKey, metadata);
+     }
+   }
+}