Part 4 of search provider refactoring.

chrome/browser/autocomplete/base_search_provider.h

 // Copyright 2014 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.
 //
 // This class contains common functionality for search-based autocomplete
 // providers. Search provider and zero suggest provider both use it for common
 // functionality.
 
 #ifndef CHROME_BROWSER_AUTOCOMPLETE_BASE_SEARCH_PROVIDER_H_
 #define CHROME_BROWSER_AUTOCOMPLETE_BASE_SEARCH_PROVIDER_H_
@@ skipping 21 matching lines @@
 // Base functionality for receiving suggestions from a search engine.
 // This class is abstract and should only be used as a base for other
 // autocomplete providers utilizing its functionality.
 class BaseSearchProvider : public AutocompleteProvider,
                            public net::URLFetcherDelegate {
  public:
   BaseSearchProvider(AutocompleteProviderListener* listener,
                      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;
 
   bool field_trial_triggered_in_session() const {
     return field_trial_triggered_in_session_;
   }
 
  protected:
   virtual ~BaseSearchProvider();
 
@@ skipping 195 matching lines @@
     // suppresses the verbatim result.
     int verbatim_relevance;
 
     // The JSON metadata associated with this server response.
     std::string metadata;
 
    private:
     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.
   //
   // |input_text| is the original user input.  This is used to highlight
   // portions of the match contents to distinguish locally-typed text from
   // suggested text.
   //
   // |input| is necessary for various other details, like whether we should
   // allow inline autocompletion and what the transition type should be.
@@ skipping 39 matching lines @@
   //   especially if we want to support suggest providers other than Google, we
   //   may change this to be a standalone setting or part of some explicit
   //   general opt-in.
   static bool CanSendURL(
       const GURL& current_page_url,
       const GURL& suggest_url,
       const TemplateURL* template_url,
       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
   // '"google:fieldtrialtriggered":true'.
   bool field_trial_triggered_;
 
   // Same as above except that it is maintained across the current Omnibox
   // session.
   bool field_trial_triggered_in_session_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(BaseSearchProvider);
 };
 
 #endif  // CHROME_BROWSER_AUTOCOMPLETE_BASE_SEARCH_PROVIDER_H_