[search_engines] Make TemplateURLService keyword map a std::multimap.

In the new world where we are no longer uniquifying keywords, and
instead tolerating multiple engines with the same keyword (we pretty
much have to), it's more convenient for us to use an std::multimap to
map keywords to engines rather than use std::map.

This is to unblock a few ideas I have:

 - Currently, to detect conflicts between engines, we traverse through
   the whole list of TemplateURLs during Add() and Update(). During
   startup and Sync, when we are adding a bunch of these engines, that
   makes the whole process O(n^2). I'd like to use this multimap to
   remove those loops (in a future CL).

 - Although it SEEMS like we may no longer need to detect conflicts in
   the new world of tolerating conflicts, that's not true. We still
   want to remove replaceable conflicting engines, and so we still want
   to detect them.

 - I also hope the multimap will enable me to unify and simplify the
   currently complicated logic that does the removal of replaceable
   conflicting engines.

 - Finally, I think the multimap won't be expensive to retrieve the
   "best" engine for any given keyword, since the typical number of
   values we linearly go through per keyword is going to be one to
   three... definitely single digits. Multimaps don't sort on value.

These things are going to be in followup CLs that are enabled by the
multimap.

See also:
https://chromium-review.googlesource.com/c/chromium/src/+/2519775

Bug: 1022775
Change-Id: I631ea6c3cf442736273e23fcda15f72095dca28a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530801
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826545}

components/search_engines/template_url_service.cc

@@ -398,9 +398,19 @@ TemplateURL* TemplateURLService::GetTemplateURLForKeyword(
 
 const TemplateURL* TemplateURLService::GetTemplateURLForKeyword(
     const base::string16& keyword) const {
-  auto elem(keyword_to_turl_and_length_.find(keyword));
-  if (elem != keyword_to_turl_and_length_.end())
-    return elem->second.first;
+  // Finds and returns the best match for |keyword|.
+  const auto match_range = keyword_to_turl_and_length_.equal_range(keyword);
+  if (match_range.first != match_range.second) {
+    // Among the matches for |keyword| in the multimap, return the best one.
+    return std::min_element(
+               match_range.first, match_range.second,
+               [](const auto& a, const auto& b) {
+                 return a.second.first
+                     ->IsBetterThanEngineWithConflictingKeyword(b.second.first);
+               })
+        ->second.first;
+  }
+
   return (!loaded_ && initial_default_search_provider_ &&
           (initial_default_search_provider_->keyword() == keyword))
              ? initial_default_search_provider_.get()
@@ -1080,7 +1090,6 @@ base::Optional<syncer::ModelError> TemplateURLService::ProcessSyncChanges(
       MaybeUpdateDSEViaPrefs(existing_turl);
   }
 
-
   // If something went wrong, we want to prematurely exit to avoid pushing
   // inconsistent data to Sync. We return the last error we received.
   if (error.IsSet())
@@ -1462,28 +1471,15 @@ void TemplateURLService::Init(const Initializer* initializers,
 
 void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) {
   const base::string16& keyword = template_url->keyword();
-  auto iter = keyword_to_turl_and_length_.find(keyword);
-  CHECK(iter != keyword_to_turl_and_length_.end());
-  // The entry at |iter| may not be |template_url| if it's an extension-created
-  // entry with the same keyword.
-  if (iter->second.first == template_url) {
-    // We need to check whether the keyword can now be provided by another
-    // TemplateURL. See the comments for BestEngineForKeyword() for more
-    // information on extension keywords and how they can coexist with
-    // non-extension keywords.
-    TemplateURL* best_fallback = nullptr;
-    for (const auto& turl : template_urls_) {
-      if ((turl.get() != template_url) && (turl->keyword() == keyword)) {
-        if (!best_fallback ||
-            turl->IsBetterThanEngineWithConflictingKeyword(best_fallback)) {
-          best_fallback = turl.get();
-        }
-      }
-    }
-    if (best_fallback) {
-      AddToMap(best_fallback);
+
+  // Remove from |keyword_to_turl_and_length_|. No need to find the best
+  // fallback. We choose the best one as-needed from the multimap.
+  const auto match_range = keyword_to_turl_and_length_.equal_range(keyword);
+  for (auto it = match_range.first; it != match_range.second;) {
+    if (it->second.first == template_url) {
+      it = keyword_to_turl_and_length_.erase(it);
     } else {
-      keyword_to_turl_and_length_.erase(iter);
+      ++it;
     }
   }
 
@@ -1499,22 +1495,13 @@ void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) {
 }
 
 void TemplateURLService::AddToMaps(TemplateURL* template_url) {
-  bool template_url_is_omnibox_api =
-      template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION;
   const base::string16& keyword = template_url->keyword();
-  KeywordToTURLAndMeaningfulLength::const_iterator i =
-      keyword_to_turl_and_length_.find(keyword);
-  if (i == keyword_to_turl_and_length_.end()) {
-    AddToMap(template_url);
-  } else {
-    TemplateURL* existing_url = i->second.first;
-    CHECK_NE(existing_url, template_url);
-    if (template_url->IsBetterThanEngineWithConflictingKeyword(existing_url)) {
-      AddToMap(template_url);
-    }
-  }
+  keyword_to_turl_and_length_.insert(std::make_pair(
+      keyword,
+      TURLAndMeaningfulLength(
+          template_url, GetMeaningfulKeywordLength(keyword, template_url))));
 
-  if (template_url_is_omnibox_api)
+  if (template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION)
     return;
 
   if (!template_url->sync_guid().empty())
@@ -1524,13 +1511,6 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) {
     provider_map_->Add(template_url, search_terms_data());
 }
 
-void TemplateURLService::AddToMap(TemplateURL* template_url) {
-  const base::string16& keyword = template_url->keyword();
-  keyword_to_turl_and_length_[keyword] =
-      TURLAndMeaningfulLength(
-          template_url, GetMeaningfulKeywordLength(keyword, template_url));
-}
-
 void TemplateURLService::SetTemplateURLs(
     std::unique_ptr<OwnedTemplateURLVector> urls) {
   Scoper scoper(this);

components/search_engines/template_url_service.h

@@ -144,7 +144,8 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Adds to |matches| all TemplateURLs whose keywords begin with |prefix|,
   // sorted shortest-keyword-first. If |supports_replacement_only| is true, only
-  // TemplateURLs that support replacement are returned.
+  // TemplateURLs that support replacement are returned. This method must be
+  // efficient, since it's run roughly once per omnibox keystroke.
   void AddMatchingKeywords(const base::string16& prefix,
                            bool supports_replacement_only,
                            TURLsAndMeaningfulLengths* matches);
@@ -461,11 +462,14 @@ class TemplateURLService : public WebDataServiceConsumer,
   using GUIDToTURL = std::map<std::string, TemplateURL*>;
 
   // A mapping from keywords to the corresponding TemplateURLs and their
-  // meaningful keyword lengths.  A keyword can appear only once here because
-  // there can be only one active TemplateURL associated with a given keyword.
+  // meaningful keyword lengths.  This is a multimap, so the system can
+  // efficiently tolerate multiple engines with the same keyword, like from
+  // extensions.  The values are not sorted from best to worst for each keyword,
+  // since multimaps don't sort on value. Users that want the best value for
+  // each key must traverse through all matching items, but we expect there to
+  // be below three values per key.
   using KeywordToTURLAndMeaningfulLength =
-      std::map<base::string16, TURLAndMeaningfulLength>;
-
+      std::multimap<base::string16, TURLAndMeaningfulLength>;
   // 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
@@ -530,7 +534,6 @@ class TemplateURLService : public WebDataServiceConsumer,
   void ApplyDefaultSearchChange(const TemplateURLData* new_dse_data,
                                 DefaultSearchManager::Source source);
 
-
   // Applies a DSE change. May be called at startup or after transitioning to
   // the loaded state. Returns true if a change actually occurred.
   bool ApplyDefaultSearchChangeNoMetrics(const TemplateURLData* new_dse_data,