Introduces a search term extraction mechanism working for arbitrary search providers.

chrome/browser/search_engines/template_url.h

 // 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.
 
 #ifndef CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_H_
 #define CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_H_
 
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/time.h"
 #include "chrome/browser/common/url_database/template_url_id.h"
 #include "googleurl/src/gurl.h"
+#include "googleurl/src/url_parse.h"
 
 class Profile;
 class SearchTermsData;
 class TemplateURL;
 
 
 // TemplateURLRef -------------------------------------------------------------
 
 // A TemplateURLRef represents a single URL within the larger TemplateURL class
 // (which represents an entire "search engine", see below).  If
 // SupportsReplacement() is true, this URL has placeholders in it, for which
 // callers can substitute values to get a "real" URL using ReplaceSearchTerms().
 //
 // TemplateURLRefs always have a non-NULL |owner_| TemplateURL, which they
 // access in order to get at important data like the underlying URL string or
 // the associated Profile.
 class TemplateURLRef {
  public:
   // Magic numbers to pass to ReplaceSearchTerms() for the |accepted_suggestion|
   // parameter.  Most callers aren't using Suggest capabilities and should just
   // pass NO_SUGGESTIONS_AVAILABLE.
   // NOTE: Because positive values are meaningful, make sure these are negative!
   enum AcceptedSuggestion {
     NO_SUGGESTION_CHOSEN = -1,
     NO_SUGGESTIONS_AVAILABLE = -2,
   };
 
   // Which kind of URL within our owner we are.  This allows us to get at the
-  // correct string field.
+  // correct string field. Use |INDEXED| to indicate that the numerical
+  // |index_in_owner_| should be used instead.

[Peter Kasting, 2012/10/02 21:47:59]

Do you think it'd be clearer to do something like this:

  enum Type {
    SEARCH = -3,
    SUGGEST = -2,
    INSTANT = -1,
    // All nonnegative values represent an alternate URL, obtainable by calling
    // GetURL(value) on the owner.
  };

This way we wouldn't have two fields, one of which is only valid sometimes, to
represent the type of URL we have.  OTOH, this might force us to add casts in
some places.

[beaudoin, 2012/10/03 22:46:52]

I'm not a fan of using enums as ints but it's true that the extra field is
annoying. I won't do it this time around but if you and/or other reviewers feel
strongly about this I'll change it.

[Peter Kasting, 2012/10/03 22:59:33]

I don't feel strongly.  Both ways have issues.  My hope was that changing to
this would shorten the code and not introduce many casts, in which case I think
it would be an improvement.  But I didn't check to see if that was actually the
case :)

[beaudoin, 2012/10/04 00:03:58]

I confess I did not actually implement it to see how much it would simplify the
code. :)

   enum Type {
     SEARCH,
     SUGGEST,
     INSTANT,
+    INDEXED
   };
 
   // This struct encapsulates arguments passed to
   // TemplateURLRef::ReplaceSearchTerms methods.  By default, only search_terms
   // is required and is passed in the constructor.
   struct SearchTermsArgs {
     explicit SearchTermsArgs(const string16& search_terms);
 
     // The search terms (query).
     const string16 search_terms;
     // The original (input) query.
     string16 original_query;
     // The optional assisted query stats, aka AQS, used for logging purposes.
     // This string contains impressions of all autocomplete matches shown
     // at the query submission time.  For privacy reasons, we require the
     // search provider to support HTTPS protocol in order to receive the AQS
     // param.
     // For more details, see http://goto.google.com/binary-clients-logging .
     std::string assisted_query_stats;
 
     // TODO: Remove along with "aq" CGI param.
     int accepted_suggestion;
   };
 
   TemplateURLRef(TemplateURL* owner, Type type);
+  TemplateURLRef(TemplateURL* owner, size_t index_in_owner);
   ~TemplateURLRef();
 
   // Returns the raw URL. None of the parameters will have been replaced.
   std::string GetURL() const;
 
   // Returns true if this URL supports replacement.
   bool SupportsReplacement() const;
 
   // Like SupportsReplacement but usable on threads other than the UI thread.
   bool SupportsReplacementUsingTermsData(
@@ skipping 38 matching lines @@
   // the key of the search term, otherwise this returns an empty string.
   const std::string& GetSearchTermKey() const;
 
   // Converts the specified term in our owner's encoding to a string16.
   string16 SearchTermToString16(const std::string& term) const;
 
   // Returns true if this TemplateURLRef has a replacement term of
   // {google:baseURL} or {google:baseSuggestURL}.
   bool HasGoogleBaseURLs() const;
 
+  // Use the alternate URLs and the search URL to match the provided |url|
+  // and extract |search_terms| from it. Returns false and an empty
+  // |search_terms| if no search terms can be matched.
+  bool ExtractSearchTermsFromURL(const GURL& url,
+                                 string16* search_terms) const;

[Peter Kasting, 2012/10/02 21:47:59]

Nit: Is it ever valid to return an empty string for |search_terms|?  If not, we
can simply return the search terms by value and callers can check for them being
non-empty to see if the extraction succeeded.

[beaudoin, 2012/10/03 22:46:52]

This is done to identify the case:
 "q=" was found but nothing after it.
Allowing us to fail on:
  http://google.com/?q=foo#q=
as was requested by a previous reviewer.

For a bit more details on why this works and why we need both the bool and the
return string, see around line 806 in template_url.cc.

[Peter Kasting, 2012/10/03 22:59:33]

Ah, I get it.  Makes  sense, but I suggest adding more comments here about
whether an empty string would be valid to extract (it is) as well as at that
callsite to explain why we want to detect empty strings and handle them
differently than we would a failed extraction.

[beaudoin, 2012/10/04 00:03:58]

Done. (Patch Set 12)

+
  private:
   friend class TemplateURL;
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, SetPrepopulatedAndParse);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseParameterKnown);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseParameterUnknown);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseURLEmpty);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseURLNoTemplateEnd);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseURLNoKnownParameters);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseURLTwoParameters);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, ParseURLNestedParameter);
@@ skipping 58 matching lines @@
   void ParseIfNecessary() const;
 
   // Like ParseIfNecessary but usable on threads other than the UI thread.
   void ParseIfNecessaryUsingTermsData(
       const SearchTermsData& search_terms_data) const;
 
   // Extracts the query key and host from the url.
   void ParseHostAndSearchTermKey(
       const SearchTermsData& search_terms_data) const;
 
+  // Extract query key and host given a list of parameters coming from the URL
+  // query or ref.
+  void FindSearchTermsKey(const std::string& params) const;
+
   // The TemplateURL that contains us.  This should outlive us.
   TemplateURL* const owner_;
 
   // What kind of URL we are.
   const Type type_;
 
+  // If |type_| is |INDEXED|, this |index_in_owner_| is used instead to refer to
+  // a url within our owner.
+  const size_t index_in_owner_;
+
   // Whether the URL has been parsed.
   mutable bool parsed_;
 
   // Whether the url was successfully parsed.
   mutable bool valid_;
 
   // The parsed URL. All terms have been stripped out of this with
   // replacements_ giving the index of the terms to replace.
   mutable std::string parsed_url_;
 
   // Do we support replacement?
   mutable bool supports_replacements_;
 
   // The replaceable parts of url (parsed_url_). These are ordered by index
   // into the string, and may be empty.
   mutable Replacements replacements_;
 
-  // Host, path and key of the search term. These are only set if the url
-  // contains one search term.
+  // Host, path, key and location of the search term. These are only set if the
+  // url contains one search term.
   mutable std::string host_;
   mutable std::string path_;
   mutable std::string search_term_key_;
+  mutable url_parse::Parsed::ComponentType search_term_key_location_;
 
   // Whether the contained URL is a pre-populated URL.
   bool prepopulated_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLRef);
 };
 
 
 // TemplateURLData ------------------------------------------------------------
 
@@ skipping 71 matching lines @@
   // the New Tab page, de-emphasizing the omnibox as "where you go to search".
   int usage_count;
 
   // If this TemplateURL comes from prepopulated data the prepopulate_id is > 0.
   int prepopulate_id;
 
   // The primary unique identifier for Sync. This set on all TemplateURLs
   // regardless of whether they have been associated with Sync.
   std::string sync_guid;
 
+  // A list of URL patterns that can be used, in addition to |url_|, to extract
+  // search terms from a URL.
+  std::vector<std::string> alternate_urls;
+
  private:
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, SerializeAlternateURLs);

[Peter Kasting, 2012/10/02 21:47:59]

This test doesn't seem to exist?

[beaudoin, 2012/10/03 22:46:52]

Good catch! (Would make for a nice presubmit check. :))

Done.

+
   // Private so we can enforce using the setters and thus enforce that these
   // fields are never empty.
   string16 keyword_;
   std::string url_;
 };
 
 
 // TemplateURL ----------------------------------------------------------------
 
 // A TemplateURL represents a single "search engine", defined primarily as a
@@ skipping 23 matching lines @@
   const string16& short_name() const { return data_.short_name; }
   // An accessor for the short_name, but adjusted so it can be appropriately
   // displayed even if it is LTR and the UI is RTL.
   string16 AdjustedShortNameForLocaleDirection() const;
 
   const string16& keyword() const { return data_.keyword(); }
 
   const std::string& url() const { return data_.url(); }
   const std::string& suggestions_url() const { return data_.suggestions_url; }
   const std::string& instant_url() const { return data_.instant_url; }
+  const std::vector<std::string>& alternate_urls() const {
+    return data_.alternate_urls;
+  }
   const GURL& favicon_url() const { return data_.favicon_url; }
 
   const GURL& originating_url() const { return data_.originating_url; }
 
   bool show_in_default_list() const { return data_.show_in_default_list; }
   // Returns true if show_in_default_list() is true and this TemplateURL has a
   // TemplateURLRef that supports replacement.
   bool ShowInDefaultList() const;
 
   bool safe_for_autoreplace() const { return data_.safe_for_autoreplace; }
@@ skipping 33 matching lines @@
   // update the keyword to reflect the current Google base URL TLD.
   bool IsGoogleSearchURLWithReplaceableKeyword() const;
 
   // Returns true if the keywords match or if
   // IsGoogleSearchURLWithReplaceableKeyword() is true for both TemplateURLs.
   bool HasSameKeywordAs(const TemplateURL& other) const;
 
   std::string GetExtensionId() const;
   bool IsExtensionKeyword() const;
 
+  // Returns the total number of URLs comprised in this template, including
+  // search and alternate URLs.
+  size_t URLCount() const;
+
+  // Obtain the URL given an |index|. Alternate URLS start at index 0, followed
+  // by the regular search URL. This allows us to prioritize some pattern, so if
+  // a search term is present both in the query and the ref, we can prioritize

[Peter Kasting, 2012/10/02 21:47:59]

Nit: Don't hardcode the idea that ref > query into the comments here.  How about
something like this instead:

  // Gets the search URL at the given index.  The alternate URLs, if any, are
  // numbered starting at 0, and the primary search URL follows.  This is used
  // to decode the search term given a search URL (see
  // ExtractSearchTermsFromURL()) and allows us to specify a priority order for
  // matching by listing alternate URLs in an appropriate order (the URL at
  // index 0 is treated as the highest priority).  For example, if a TemplateURL
  // has alternate URL "http://foo/#q={searchTerms}" and search URL
  // "http://foo/?q={searchTerms}", and the URL to be decoded is
  // "http://foo/?q=a#q=b", the alternate URL will match first and the decoded
  // search term will be "b".

[beaudoin, 2012/10/03 22:46:52]

Shuffled your proposal a bit between this and ExtractSearchTermsFromURL.

Done.

+  // the ref one. The |index| must be less than URLCount().
+  const std::string& GetURL(size_t index) const;
+
+  // Use the alternate URLs and the search URL to match the provided |url|
+  // and extract |search_terms| from it. Returns false and an empty
+  // |search_terms| if no search terms can be matched.

[Peter Kasting, 2012/10/02 21:47:59]

Nit: Add note about the matching priority, perhaps by simply referencing the
comments above.

[beaudoin, 2012/10/03 22:46:52]

Done.

+  bool ExtractSearchTermsFromURL(const GURL& url, string16* search_terms);
+
  private:
   friend class TemplateURLService;
 
   void CopyFrom(const TemplateURL& other);
 
   void SetURL(const std::string& url);
   void SetPrepopulateId(int id);
 
   // Resets the keyword if IsGoogleSearchURLWithReplaceableKeyword() or |force|.
   // The |force| parameter is useful when the existing keyword is known to be
   // a placeholder.  The resulting keyword is generated using
   // TemplateURLService::GenerateSearchURL() and
   // TemplateURLService::GenerateKeyword().
   void ResetKeywordIfNecessary(bool force);
 
   Profile* profile_;
   TemplateURLData data_;
   TemplateURLRef url_ref_;
   TemplateURLRef suggestions_url_ref_;
   TemplateURLRef instant_url_ref_;
 
   // TODO(sky): Add date last parsed OSD file.
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURL);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_H_