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

chrome/browser/search_engines/template_url.h

Index: chrome/browser/search_engines/template_url.h
diff --git a/chrome/browser/search_engines/template_url.h b/chrome/browser/search_engines/template_url.h
index 025b88e31eb43dce7097799a84ab878fe5c1ad99..8f4fd454782e824a26174c236b728e3fe9ff8da3 100644
--- a/chrome/browser/search_engines/template_url.h
+++ b/chrome/browser/search_engines/template_url.h
@@ -12,6 +12,7 @@
 #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;
@@ -40,11 +41,13 @@ class TemplateURLRef {
   };
 
   // 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
@@ -70,6 +73,7 @@ class TemplateURLRef {
   };
 
   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.
@@ -128,6 +132,12 @@ class TemplateURLRef {
   // {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);
@@ -206,12 +216,20 @@ class TemplateURLRef {
   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_;
 
@@ -229,11 +247,12 @@ class TemplateURLRef {
   // 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_;
@@ -325,7 +344,13 @@ struct TemplateURLData {
   // 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_;
@@ -369,6 +394,9 @@ class TemplateURL {
   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; }
@@ -422,6 +450,21 @@ class TemplateURL {
   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;