[search_engines] Stop trying to prevent keyword conflicts in Update()

We're removing the invariant that two non-extension search engines
cannot share the same keyword.

This invariant was untenable because of Sync, Play API, and Policy.
It was also kind of a weak invariant because extensions have always
been able to create duplicate keywords.

The invariant was costly, because it was a big source of duplicate
search engines caused by appending underscores to uniquify keywords.

And the invariant was not needed, because AddToMaps() already handles
duplicate keywords by choosing the best engine, since extensions have
always been able to introduce duplicate keywords.

This CL changes the Update() method to no longer attempt to keep
non-extension engine keywords unique, for a couple of reasons:

 1. Simplify the code.

 2. The conflict avoidance logic only worked as long as we keep the
    invariant true, and we're removing the invariant. Concretely,
    we plan on having Add() no longer uniquify keywords, and this
    logic crashes in that world. See:
    https://chromium-review.googlesource.com/c/chromium/src/+/2515679

 3. Having Update() sometimes silently fail to update the keyword
    actually seems a bit strange to me, and is potentially a source of
    Sync conflicts, since two clients could disagree on whether they
    update the keyword or not.

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

components/search_engines/template_url_service.cc

@@ -1643,29 +1643,28 @@ bool TemplateURLService::Update(TemplateURL* existing_turl,
   Scoper scoper(this);
   model_mutated_notification_pending_ = true;
 
-  base::string16 old_keyword = existing_turl->keyword();
   TemplateURLID previous_id = existing_turl->id();
   RemoveFromMaps(existing_turl);
 
-  // Check if new keyword conflicts with another normal engine.
+  // Check for new keyword conflicts with another normal engine.
   // This is possible when autogeneration of the keyword for a Google default
   // search provider at load time causes it to conflict with an existing
-  // keyword. In this case we delete the existing keyword if it's replaceable,
-  // or else undo the change in keyword for |existing_turl|.
-  // Conflicts with extension engines are handled in AddToMaps/RemoveFromMaps
-  // functions.
-  // Search for conflicting keyword turl before updating values of
-  // existing_turl.
-  const TemplateURL* conflicting_keyword_turl =
-      FindNonExtensionTemplateURLForKeyword(new_values.keyword());
-
-  bool keep_old_keyword = false;
-  if (conflicting_keyword_turl && conflicting_keyword_turl != existing_turl) {
-    if (CanReplace(conflicting_keyword_turl))
-      Remove(conflicting_keyword_turl);
-    else
-      keep_old_keyword = true;
+  // keyword. If the conflicting engines are replaceable, we delete them.
+  // If they're not replaceable, we leave them alone, and trust AddToMaps() to
+  // choose the best engine to assign the keyword.
+  std::vector<TemplateURL*> turls_to_remove;
+  for (const auto& turl : template_urls_) {
+    // TODO(tommycli): Investigate also replacing TemplateURL::LOCAL engines.
+    if (turl.get() != existing_turl && (turl->type() == TemplateURL::NORMAL) &&
+        (turl->keyword() == new_values.keyword()) && CanReplace(turl.get())) {
+      // Remove() invalidates iterators.
+      turls_to_remove.push_back(turl.get());
+    }
+  }
+  for (TemplateURL* turl : turls_to_remove) {
+    Remove(turl);
   }
+
   // Update existing turl with new values. This must happen after calling
   // Remove(conflicting_keyword_turl) above, since otherwise during that
   // function RemoveFromMaps() may find |existing_turl| as an alternate engine
@@ -1675,10 +1674,6 @@ bool TemplateURLService::Update(TemplateURL* existing_turl,
   // calling AddToMaps() below).
   existing_turl->CopyFrom(new_values);
   existing_turl->data_.id = previous_id;
-  if (keep_old_keyword) {
-    CHECK_NE(old_keyword, new_values.keyword());
-    existing_turl->data_.SetKeyword(old_keyword);
-  }
 
   AddToMaps(existing_turl);
 
@@ -1696,7 +1691,6 @@ bool TemplateURLService::Update(TemplateURL* existing_turl,
   if (default_search_provider_source_ != DefaultSearchManager::FROM_FALLBACK)
     MaybeUpdateDSEViaPrefs(existing_turl);
 
-  CHECK(!HasDuplicateKeywords());
   return true;
 }
 
@@ -1974,7 +1968,6 @@ TemplateURL* TemplateURLService::Add(std::unique_ptr<TemplateURL> template_url,
   if (template_url_ptr)
     model_mutated_notification_pending_ = true;
 
-  CHECK(!HasDuplicateKeywords());
   return template_url_ptr;
 }
 
@@ -2327,18 +2320,3 @@ TemplateURL* TemplateURLService::FindMatchingDefaultExtensionTemplateURL(
   }
   return nullptr;
 }
-
-bool TemplateURLService::HasDuplicateKeywords() const {
-  std::map<base::string16, TemplateURL*> keyword_to_template_url;
-  for (const auto& template_url : template_urls_) {
-    // Validate no duplicate normal engines with same keyword.
-    if (!IsCreatedByExtension(template_url.get())) {
-      if (keyword_to_template_url.find(template_url->keyword()) !=
-          keyword_to_template_url.end()) {
-        return true;
-      }
-      keyword_to_template_url[template_url->keyword()] = template_url.get();
-    }
-  }
-  return false;
-}

components/search_engines/template_url_service.h

@@ -718,11 +718,6 @@ class TemplateURLService : public WebDataServiceConsumer,
   TemplateURL* FindMatchingDefaultExtensionTemplateURL(
       const TemplateURLData& data);
 
-  // Returns whether |template_urls_| contains more than one normal engine with
-  // same keyword. Used to validate state after search engines are
-  // added/updated.
-  bool HasDuplicateKeywords() const;
-
   // ---------- Browser state related members ---------------------------------
   PrefService* prefs_ = nullptr;