Changed keywords conflicts resolution for extensions search engines.

components/search_engines/template_url_service.cc

Index: components/search_engines/template_url_service.cc
diff --git a/components/search_engines/template_url_service.cc b/components/search_engines/template_url_service.cc
index 0e77d27379f265169564fc3bb4cf7eff964c7a4f..70a97910235e9f646ed44a8c815ade270534ce6c 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -2015,7 +2015,7 @@ TemplateURL* TemplateURLService::AddNoNotify(
   template_url->ResetKeywordIfNecessary(search_terms_data(), false);
   // Check whether |template_url|'s keyword conflicts with any already in the
   // model.
-  TemplateURL* existing_keyword_turl =
+  TemplateURL* existing_turl =
       GetTemplateURLForKeyword(template_url->keyword());
 
   // Check whether |template_url|'s keyword conflicts with any already in the
@@ -2028,24 +2028,37 @@ TemplateURL* TemplateURLService::AddNoNotify(
   // |template_urls_|, lest we detect a "conflict" between the
   // |initial_default_search_provider_| and the web data version of itself.
   if (template_url->type() != TemplateURL::OMNIBOX_API_EXTENSION &&
-      existing_keyword_turl &&
-      existing_keyword_turl->type() != TemplateURL::OMNIBOX_API_EXTENSION &&
-      Contains(&template_urls_, existing_keyword_turl)) {
-    DCHECK_NE(existing_keyword_turl, template_url.get());
+      existing_turl &&
+      existing_turl->type() != TemplateURL::OMNIBOX_API_EXTENSION &&
+      Contains(&template_urls_, existing_turl)) {
+    DCHECK_NE(existing_turl, template_url.get());
     // Only replace one of the TemplateURLs if they are either both extensions,
     // or both not extensions.
-    bool are_same_type = IsCreatedByExtension(existing_keyword_turl) ==
+    bool are_same_type = IsCreatedByExtension(existing_turl) ==
                          IsCreatedByExtension(template_url.get());
-    if (CanReplace(existing_keyword_turl) && are_same_type) {
-      RemoveNoNotify(existing_keyword_turl);
+    if (CanReplace(existing_turl) && are_same_type) {
+      RemoveNoNotify(existing_turl);
     } else if (CanReplace(template_url.get()) && are_same_type) {
       return nullptr;
     } else {
-      base::string16 new_keyword =
-          UniquifyKeyword(*existing_keyword_turl, false);
-      ResetTemplateURLNoNotify(existing_keyword_turl,
-                               existing_keyword_turl->short_name(), new_keyword,
-                               existing_keyword_turl->url());
+      // Resolve keyword conflicts for engines that can not be replaced.
+      // Use following priority while resolving keyword conflicts:
+      // extension default search -> extension search -> normal engines.
+      // For engines of the same priority, the newly-added one wins.
+      // By extension we mean NORMAL_CONTROLLED_BY_EXTENSION type of engine, not
+      // the OMNIBOX_API_EXTENSION.

[vasilii, 2017/02/07 16:27:21]

I'm confused by the algorithm because I thought we agreed on something
different: Extension keywords are not replaced even between each other. The
later one wins. Not sure why |wants_to_be_default_engine| matters.

With the current approach you make FindMatchingExtensionTemplateURL from
https://codereview.chromium.org/2639153002 more reliable but the failure is
still possible (when you have two extension DSEs added in the reverse order
here).

[Alexander Yashkin, 2017/02/07 16:50:11]

I have understood differently, especially Peter's comment "Right, the conflict
resolution algorithm here is clearly wrong.  No extension keyword should be ever
changed, and that goes double for an extension keyword set as default."

It seems strange to me if non default extension could change keyword of default
extension even if installed later.

And what do you mean - "Extension keywords are not replaced even between each
other." How its possible to have two extensions installed with same keywords? Do
you mean to evict such engine from TemplateURLService?
I am aware of it, as I already have written to Peter - 
"Also storing extension_id, IMHO, anyway is better and more clear way of
choosing extension default search, 
because choosing engine by matching TemplateURLData can in exotic cases (2
extensions with same engines) match to engine from another extension
or fail to match at all as I accidentally encountered in the case above."

I will readily change my solution if you propose the algorithm that looks better
to you.

+      base::string16 new_keyword = UniquifyKeyword(*existing_turl, false);
+      if (IsCreatedByExtension(existing_turl) &&
+          (!IsCreatedByExtension(template_url.get()) ||
+           (existing_turl->extension_info_->wants_to_be_default_engine &&
+            !template_url->extension_info_->wants_to_be_default_engine))) {
+        // Replace keyword for added search engine.
+        template_url->data_.SetKeyword(new_keyword);
+      } else {
+        // Replace keyword for existing engine.
+        ResetTemplateURLNoNotify(existing_turl, existing_turl->short_name(),
+                                 new_keyword, existing_turl->url());
+      }
     }
   }
   TemplateURL* template_url_ptr = template_url.get();