Changed keywords conflicts resolution for extensions search engines.

components/search_engines/template_url_service.cc

 // 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.
 
 #include "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 1997 matching lines @@
 
   if (newly_adding) {
     DCHECK_EQ(kInvalidTemplateURLID, template_url->id());
     DCHECK(!Contains(&template_urls_, template_url.get()));
     template_url->data_.id = ++next_id_;
   }
 
   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
   // model.  Note that we can reach here during the loading phase while
   // processing the template URLs from the web data service.  In this case,
   // GetTemplateURLForKeyword() will look not only at what's already in the
   // model, but at the |initial_default_search_provider_|.  Since this engine
   // will presumably also be present in the web data, we need to double-check
   // that any "pre-existing" entries we find are actually coming from
   // |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();
   template_urls_.push_back(std::move(template_url));
   AddToMaps(template_url_ptr);
 
   if (newly_adding && (template_url_ptr->type() == TemplateURL::NORMAL)) {
     if (web_data_service_.get())
       web_data_service_->AddKeyword(template_url_ptr->data());
 
@@ skipping 418 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }