Omnibox: Make Keyword Provide More Generous with Matching

components/search_engines/template_url_service.h

Index: components/search_engines/template_url_service.h
diff --git a/components/search_engines/template_url_service.h b/components/search_engines/template_url_service.h
index f77879cdab5ed455f5be883a18e9390dd9fd5ee5..3e61efcf19b5826e62180e80971bec84ba0a55d0 100644
--- a/components/search_engines/template_url_service.h
+++ b/components/search_engines/template_url_service.h
@@ -76,6 +76,13 @@ class TemplateURLService : public WebDataServiceConsumer,
   typedef std::vector<TemplateURL*> TemplateURLVector;
   typedef std::map<std::string, syncer::SyncData> SyncDataMap;
   typedef base::CallbackList<void(void)>::Subscription Subscription;
+  // Under some matching regimes, we may want to treat the keyword in a
+  // TemplateURL as being a different length than it actually is.  (There
+  // may be parts of the keyword that we consider not required and don't
+  // want to penalize for the user not typing those.)

[Peter Kasting, 2015/11/09 23:11:15]

Nit: How about: "For keywords that end in a TLD, e.g 'foo.com', we want to
record that the pre-TLD portion is more meaningful, to allow matching against
just that portion of the keyword in some scenarios."

[Mark P, 2015/11/11 07:41:01]

I find this confusing.  I don't know what your explanation means, and the parts
I do understand seem wrong.  I rewrote my comment to be a little more like your
suggestion.

+  typedef std::pair<TemplateURL*, size_t> TemplateURLAndEffectiveKeywordLength;

[Peter Kasting, 2015/11/09 23:11:16]

Nit: This is excessively verbose; call this TURLAndEffectiveLength

[Mark P, 2015/11/11 07:41:01]

Done, except used Meaningful, not Effective, per your other comment.

+  typedef std::vector<TemplateURLAndEffectiveKeywordLength>
+      TemplateURLMatchesVector;

[Peter Kasting, 2015/11/09 23:11:15]

Nit: Don't put "Vector" in the typedef name, and "matches" is somewhat confusing
because it implies a usage (that this type can only be used for "matching"
something) rather than just being descriptive of what's in it.  If you rename
according to my suggestion above, this could be "TURLsAndEffectiveLengths"
instead.

[Mark P, 2015/11/11 07:41:01]

Makes sense to me.  Done (except using meaningful, not effective, as above).

 
   // Struct used for initializing the data store with fake data.
   // Each initializer is mapped to a TemplateURL.
@@ -125,7 +132,18 @@ class TemplateURLService : public WebDataServiceConsumer,
   // TemplateURLs that support replacement are returned.
   void FindMatchingKeywords(const base::string16& prefix,
                             bool support_replacement_only,
-                            TemplateURLVector* matches);
+                            TemplateURLMatchesVector* matches);
+
+  // Returns (in |matches|) all TemplateURLs for auto-generated search engines
+  // with the domain name part of the keyword starts with |prefix|, sorted

[Peter Kasting, 2015/11/09 23:11:15]

Nit: with -> where

Be careful using "domain name".  It doesn't just mean the TLD+1; "abc.def.com"
is a domain name, as is "def.com", as is "com" (but only the first two are also
hostnames).  For this reason I try to use "TLD+1"  pretty consistently for what
you refer to as "domain name".  The only problem with that is that it's hard to
use in a type or variable name, so some creativity is required, and in the limit
I might stoop to "domain" in such a name as long as the comments were clearer.

[Mark P, 2015/11/11 07:41:01]

Done.
I'd like to stick with domain name for consistency with the underlying functions
I'm using from
net/base/registry_controlled_domains/registry_controlled_domain.h

If those functions talked about TLD+1 (in the name or comments), I'd change
these comments appropriately.  But for now if I think the comments should
imply/hint at the implementation, that I'm using GetDomainAndRegistry() and not
something else.

+  // shortest domain-name-first. If |support_replacement_only| is true, only

[Peter Kasting, 2015/11/09 23:11:15]

Nit: Also hyphen after "shortest".  Also we should probably rename
"support_replacement_only" to "supports_replacement_only" here and elsewhere for
grammar reasons.

[Mark P, 2015/11/11 07:41:01]

Done.

Also we should probably rename
Done throughout this file and its .cc.

+  // TemplateURLs that support replacement are returned.  Does not bother
+  // searching/returning keywords that would've been found with an identical
+  // call to FindMatchingKeywords(); i.e., doesn't search keywords for which
+  // the domain name is the keyword.
+  void FindMatchingDomainKeywords(const base::string16& prefix,
+                                  bool support_replacement_only,
+                                  TemplateURLMatchesVector* matches);
 
   // Looks up |keyword| and returns the element it maps to.  Returns NULL if
   // the keyword was not found.
@@ -382,8 +400,11 @@ class TemplateURLService : public WebDataServiceConsumer,
   friend class InstantUnitTestBase;
   friend class TemplateURLServiceTestUtil;
 
-  typedef std::map<base::string16, TemplateURL*> KeywordToTemplateMap;
   typedef std::map<std::string, TemplateURL*> GUIDToTemplateMap;
+  typedef std::map<base::string16, TemplateURLAndEffectiveKeywordLength>

[Peter Kasting, 2015/11/09 23:11:15]

Nit: The Chromium C++11 guide says to prefer type alias syntax ("using") for
typedefs at this point, so maybe make all these use that?

[Mark P, 2015/11/11 07:41:01]

Revised this whole .h file accordingly.

+      KeywordToTemplateMap;
+  typedef std::multimap<base::string16, TemplateURLAndEffectiveKeywordLength>
+      KeywordDomainToTemplateMap;

[Peter Kasting, 2015/11/09 23:11:15]

Nit: Add comments to describe each of these maps, especially for the multimap to
be clear why it can have multiple results for the same key.

I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map to a
TURLAndEffectiveLength; it seems like these should have different names, but I
don't know what, and I'm hoping the comments will help make the name more
obvious.

In some cases this may just mean moving up the comments from the member
variables to these types.

[Mark P, 2015/11/11 07:41:01]

Done.
I can simply do KeywordToTURLAndEffectiveLength and
KeywordDomainToTURLAndEffectiveLength instead of what's currently here.  Those
seems clear to me.  What do you think?
              
Yup, that's what I did (moved all the content up, slightly revised it).

[Peter Kasting, 2015/11/12 20:36:10]

Seems OK to me.

[Peter Kasting, 2015/11/12 22:32:11]

You didn't make these changes.

[Mark P, 2015/11/12 23:18:41]

Now done.  Also changed the names of the internal variables correspondingly. 
Also did the same with GUIDToTemplateMap and its variable, which both seemed
not-parallel and bothered me.

 
   // Declaration of values to be used in an enumerated histogram to tally
   // changes to the default search provider from various entry points. In
@@ -424,6 +445,10 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   void AddToMaps(TemplateURL* template_url);
 
+  void RemoveFromDomainMap(const TemplateURL* template_url);
+
+  void AddToDomainMap(TemplateURL* template_url);

[Peter Kasting, 2015/11/09 23:11:15]

Nit: Please add comments for all these functions while you're here, especially
since we now have several different kinds of maps.

[Mark P, 2015/11/12 20:13:03]

Done.  (Added comments to all these Add/Remove functions.)

+
   // Sets the keywords. This is used once the keywords have been loaded.
   // This does NOT notify the delegate or the database.
   //
@@ -608,6 +633,17 @@ class TemplateURLService : public WebDataServiceConsumer,
   // |template_urls| on exit.
   void AddTemplateURLs(TemplateURLVector* template_urls);
 
+  // Returns (in |matches|) all TemplateURLs stored in |keyword_to_template_map|
+  // whose keywords begin with |prefix|, sorted shortest keyword-first.  If

[Peter Kasting, 2015/11/09 23:11:16]

Nit: Also hyphen after "shortest"

[Mark P, 2015/11/11 07:41:01]

Done.

+  // |support_replacement_only| is true, only TemplateURLs that support

[Peter Kasting, 2015/11/09 23:11:16]

Nit: support_ -> supports_

[Mark P, 2015/11/11 07:41:01]

Done.

+  // replacement are returned.
+  template <typename Container>
+  void FindMatchingKeywordsHelper(
+      const Container& keyword_to_template_map,
+      const base::string16& prefix,
+      bool support_replacement_only,
+      TemplateURLMatchesVector* matches);
+
   // Returns the TemplateURL corresponding to |prepopulated_id|, if any.
   TemplateURL* FindPrepopulatedTemplateURL(int prepopulated_id);
 
@@ -650,6 +686,19 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Mapping from keyword to the TemplateURL.
   KeywordToTemplateMap keyword_to_template_map_;
+  // Mapping from keyword domain to the TemplateURL.  Precisely, for

[Peter Kasting, 2015/11/09 23:11:15]

Nit: Blank line above

[Mark P, 2015/11/11 07:41:01]

Done.

+  // auto-generated keywords such as abc.def.com, we put the keyword in this
+  // map under the name def.com.  This is a multimap because multiple keywords
+  // can have the same domain.  Entries are only allowed here if there is a
+  // corresponding entry in |keyword_to_template_map_|, i.e., if a template URL
+  // doesn't have an entry in |keyword_to_template_map_| because it's subsumed
+  // by another template URL with an identical keyword, the template URL will
+  // not have an entry in this map either.  This map will also not bother
+  // including entries for auto-generated keywords in which the keyword is the
+  // domain name, no host before the domain name.  (The ordinary
+  // |keyword_to_template_map_| suffices for that.)  Also, non-auto-generated
+  // keywords will not have entries in this map.

[Peter Kasting, 2015/11/09 23:11:16]

How about this comment:

Mapping from TLD+1s to corresponding TemplateURLs.  Specifically, for an
autogenerated keyword that is a hostname containing more than just a TLD+1, e.g.
'abc.def.com', the keyword is added to this map under the TLD+1 'def.com'.  This
means multiple keywords from the same TLD+1 share the same key, so this must be
a multimap.

If you're moving this comment up to the typedef above, then I would leave out
the remaining details about autogenerated keywords and the like.  Then on the
comment on this particular data member, you could include that sort of thing
along with the motivation for it; you could even name the member accordingly,
e.g. "domain_to_autogenerated_turl_map_".  But, see also comments in the source
file about this whole concept.

[Mark P, 2015/11/11 07:41:01]

Done.  Used the first paragraph of the comment by moving it above; kept the
nitty-gritty details near the variable declaration.
I'm fine renaming this.  We can do domain_to_turl_ or
domain_to_turl_and_meaningful_length_.  Whether either of these is good depends
on what you say about the type name above.

Whatever we chose, the earlier variable I think should simply be this variable
without the "domain_" portion.

+  KeywordDomainToTemplateMap keyword_domain_to_template_map_;
 
   // Mapping from Sync GUIDs to the TemplateURL.
   GUIDToTemplateMap guid_to_template_map_;