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 2023 matching lines @@
     DCHECK_NE(existing_keyword_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) ==
                          IsCreatedByExtension(template_url.get());
     if (CanReplace(existing_keyword_turl) && are_same_type) {
       RemoveNoNotify(existing_keyword_turl);
     } else if (CanReplace(template_url.get()) && are_same_type) {
       return nullptr;
     } else {
+      // Resolve keyword conflicts for engines that can not be replaced.
+      // Use following priority while resolving keyword conflicts:
+      // extension default search -> extension search -> normal engines.
+      // By extension we mean NORMAL_CONTROLLED_BY_EXTENSION type of engine, not
+      // the OMNIBOX_API_EXTENSION.

[Peter Kasting, 2017/02/07 00:42:45]

Nit: Might want to say "for engines of the same priority, the newly-added one
wins."

[Alexander Yashkin, 2017/02/07 08:09:20]

Done

+      bool new_engine_is_default_extension =
+          IsCreatedByExtension(template_url.get()) &&
+          template_url->extension_info_->wants_to_be_default_engine;
+
+      bool old_engine_is_default_extension =
+          IsCreatedByExtension(existing_keyword_turl) &&
+          existing_keyword_turl->extension_info_->wants_to_be_default_engine;
+
+      // The only way we allow old engine to hold its keyword, is if it was
+      // extension installed default engine and new engine is not. And the
+      // second case is if new added engine is normal and old was from
+      // extension.
+      bool new_engine_lose = (old_engine_is_default_extension &&
+                              !new_engine_is_default_extension) ||
+                             (IsCreatedByExtension(existing_keyword_turl) &&
+                              !IsCreatedByExtension(template_url.get()));
+
       base::string16 new_keyword =
           UniquifyKeyword(*existing_keyword_turl, false);
-      ResetTemplateURLNoNotify(existing_keyword_turl,
-                               existing_keyword_turl->short_name(), new_keyword,
-                               existing_keyword_turl->url());
+      if (new_engine_lose) {

[Peter Kasting, 2017/02/07 00:42:45]

Nit: If we just do this here:

      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))) {

...then we can eliminate basically all the new code above.  (But keep the
comment at the top).

Note that for better line wrapping, I renamed |existing_keyword_turl| to
|existing_turl|, which doesn't seem less clear in context.

[Alexander Yashkin, 2017/02/07 08:09:20]

Done, hope it would be clear to next generations :).

+        // Replace keyword for added search engine.
+        template_url->data_.SetKeyword(new_keyword);
+      } else {
+        // Replace keyword for existing engine.
+        ResetTemplateURLNoNotify(existing_keyword_turl,
+                                 existing_keyword_turl->short_name(),
+                                 new_keyword, existing_keyword_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();
   }
 }