Part 4 of search provider refactoring.

chrome/browser/autocomplete/base_search_provider.h

Index: chrome/browser/autocomplete/base_search_provider.h
diff --git a/chrome/browser/autocomplete/base_search_provider.h b/chrome/browser/autocomplete/base_search_provider.h
index 41547b28032f390250128617291665db755dfeb3..a1fbff39de09bed370547e290770faf1d4528b04 100644
--- a/chrome/browser/autocomplete/base_search_provider.h
+++ b/chrome/browser/autocomplete/base_search_provider.h
@@ -39,6 +39,9 @@ class BaseSearchProvider : public AutocompleteProvider,
                      Profile* profile,
                      AutocompleteProvider::Type type);
 
+  // Returns whether |match| is flagged as a query that should be prefetched.
+  static bool ShouldPrefetch(const AutocompleteMatch& match);
+
   // AutocompleteProvider:
   virtual void AddProviderInfo(ProvidersInfo* provider_info) const OVERRIDE;
 
@@ -254,6 +257,26 @@ class BaseSearchProvider : public AutocompleteProvider,
     DISALLOW_COPY_AND_ASSIGN(Results);
   };
 
+  // The following keys are used to record additional information on matches.

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

nit: I think static variables are supposed to go after functions and before
normal variables in the class definition.

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

Actually, according to Google style guide, the order is

Typedefs and Enums
Constants (static const data members)
Constructors
Destructor
Methods, including static methods
Data Members (except static const data members)

So I am moving this up higher.

+
+  // We annotate our AutocompleteMatches with whether their relevance scores
+  // were server-provided using this key in the |additional_info| field.
+  static const char kRelevanceFromServerKey[];
+
+  // Indicates whether the server said a match should be prefetched.
+  static const char kShouldPrefetchKey[];
+
+  // Used to store metadata from the server response, which is needed for
+  // prefetching.
+  static const char kSuggestMetadataKey[];
+
+  // Used to store a deletion request url for server-provided suggestions.
+  static const char kDeletionUrlKey[];
+
+  // These are the values for the above keys.
+  static const char kTrue[];
+  static const char kFalse[];
+
   // Returns an AutocompleteMatch with the given |autocomplete_provider|
   // for the search |suggestion|, which represents a search via |template_url|.
   // If |template_url| is NULL, returns a match with an invalid destination URL.
@@ -313,6 +336,22 @@ class BaseSearchProvider : public AutocompleteProvider,
       AutocompleteInput::PageClassification page_classification,
       Profile* profile);
 
+  // Creates an AutocompleteMatch from |result| to search |template_url| for
+  // |query_string|. |input|, |accepted_suggestion|, and
+  // |default_search_provider_result| are used for generating an
+  // AutocompleteMatch using CreateSearchSuggestion(). |metadata| is added to
+  // the match for prefetch queries only.
+  // Adds this match to |map|; if such a match already exists, whichever one
+  // has lower relevance is eliminated.
+  void AddMatchToMap(const SuggestResult& result,

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

This comment is somewhat backwards, as is the parameter list.

One, the main effect of this is adding a match to |map|.  That should be in the
first or second sentence of the big comment.

Two, especially given the explanation, |template_url| is almost as important a
parameter as |result|.  It should go earlier in the list, certainly earlier than
those parameters only used for extra/side-channel information.

Three, the comment does not mention |query_string|
Also, calling this variable |query_string| is extremely misleading because it's
not the query string.  The query string is stored in |result|.  This is the
input text, sent separately from |input|.  See comment in .cc for more about
this.

Four, I don't like the comment about |meta_data|.  First, isn't this comment
likely to become obsolete (prefetch only)?  Maybe you can make a broader
statement that will likely stand the test of time.  (Can you simply add it to
the list of things merely passed onto CreateSearchSuggestion()?)  As it is, the
comment invites more questions.  If you don't want to make a broader statement,
then you need to explain what |metadata| is meant to represent and why is it
only used for prefetch queries?

Five, having a parameter here called |default_search_provider_result| be called
|append_extra_query_params| in the implementation is very confusing.  Please
make them correspond.
Also note that |default_search_provider_result| naturally reads as "is this
result from the default search provider", which is an incorrect interpretation.

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

Done.

+                     const AutocompleteInput input,

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

nit: why not &?

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

Done.

+                     const base::string16& query_string,
+                     const TemplateURL* template_url,
+                     const std::string& metadata,
+                     int accepted_suggestion,
+                     bool default_search_provider_result,
+                     MatchMap* map);
+
   // Whether a field trial, if any, has triggered in the most recent
   // autocomplete query. This field is set to true only if the suggestion
   // provider has completed and the response contained