Omnibox: Make Keyword Provide More Generous with Matching

components/search_engines/template_url_service.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.
 
 #ifndef COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 #define COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 
 #include <list>
 #include <map>
 #include <set>
@@ skipping 58 matching lines @@
 // otherwise TemplateURLService handles deletion.
 
 class TemplateURLService : public WebDataServiceConsumer,
                            public KeyedService,
                            public syncer::SyncableService {
  public:
   typedef std::map<std::string, std::string> QueryTerms;
   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.
   struct Initializer {
     const char* const keyword;
     const char* const url;
     const char* const content;
   };
 
   struct URLVisitedDetails {
@@ skipping 29 matching lines @@
   // a keyword for hosts already associated with a manually-edited keyword.
   bool CanAddAutogeneratedKeyword(const base::string16& keyword,
                                   const GURL& url,
                                   TemplateURL** template_url_to_replace);
 
   // Returns (in |matches|) all TemplateURLs whose keywords begin with |prefix|,
   // sorted shortest keyword-first. If |support_replacement_only| is true, only
   // 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.
   // The caller should not try to delete the returned pointer; the data store
   // retains ownership of it.
   TemplateURL* GetTemplateURLForKeyword(const base::string16& keyword);
 
   // Returns that TemplateURL with the specified GUID, or NULL if not found.
   // The caller should not try to delete the returned pointer; the data store
   // retains ownership of it.
@@ skipping 236 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
                            IsLocalTemplateURLBetter);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
                            ResolveSyncKeywordConflict);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, PreSyncDeletes);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, MergeInSyncTemplateURL);
 
   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
   // particular, we use this to see what proportion of changes are from Sync
   // entry points, to help spot erroneous Sync activity.
   enum DefaultSearchChangeOrigin {
     // Various known Sync entry points.
     DSP_CHANGE_SYNC_PREF,
     DSP_CHANGE_SYNC_ADD,
     DSP_CHANGE_SYNC_DELETE,
@@ skipping 20 matching lines @@
   // Helper functor for FindMatchingKeywords(), for finding the range of
   // keywords which begin with a prefix.
   class LessWithPrefix;
 
   void Init(const Initializer* initializers, int num_initializers);
 
   void RemoveFromMaps(TemplateURL* template_url);
 
   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.
   //
   // This transfers ownership of the elements in |urls| to |this|, and may
   // delete some elements, so it's not safe for callers to access any elements
   // after calling; to reinforce this, this function clears |urls| on exit.
   void SetTemplateURLs(TemplateURLVector* urls);
 
   // Transitions to the loaded state.
   void ChangeToLoadedState();
@@ skipping 164 matching lines @@
   void OnSyncedDefaultSearchProviderGUIDChanged();
 
   // Adds |template_urls| to |template_urls_|.
   //
   // This transfers ownership of the elements in |template_urls| to |this|, and
   // may delete some elements, so it's not safe for callers to access any
   // elements after calling; to reinforce this, this function clears
   // |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);
 
   // Returns the TemplateURL associated with |extension_id|, if any.
   TemplateURL* FindTemplateURLForExtension(const std::string& extension_id,
                                            TemplateURL::Type type);
 
   // Finds the extension-supplied TemplateURL that matches |data|, if any.
   TemplateURL* FindMatchingExtensionTemplateURL(const TemplateURLData& data,
                                                 TemplateURL::Type type);
@@ skipping 22 matching lines @@
   rappor::RapporService* rappor_service_;
 
   // This closure is run when the default search provider is set to Google.
   base::Closure dsp_change_callback_;
 
 
   PrefChangeRegistrar pref_change_registrar_;
 
   // 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_;
 
   TemplateURLVector template_urls_;
 
   base::ObserverList<TemplateURLServiceObserver> model_observers_;
 
   // Maps from host to set of TemplateURLs whose search url host is host.
   // NOTE: This is always non-NULL; we use a scoped_ptr<> to avoid circular
@@ skipping 66 matching lines @@
 
   // Helper class to manage the default search engine.
   DefaultSearchManager default_search_manager_;
 
   scoped_ptr<GoogleURLTracker::Subscription> google_url_updated_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };
 
 #endif  // COMPONENTS_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_