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 be5527fc553acb051431dd0a742b3f7a3cf3827c..31a53b88f36d4c461e69cab056eb32ab06f4259c 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -188,7 +188,7 @@ bool Contains(TemplateURLService::OwnedTemplateURLVector* template_urls,
   return FindTemplateURL(template_urls, turl) != template_urls->end();
 }
 
-bool IsCreatedByExtension(TemplateURL* template_url) {
+bool IsCreatedByExtension(const TemplateURL* template_url) {
   return template_url->type() ==
              TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION ||
          template_url->type() == TemplateURL::OMNIBOX_API_EXTENSION;
@@ -530,10 +530,11 @@ void TemplateURLService::RemoveAutoGeneratedForUrlsBetween(
 }
 
 void TemplateURLService::RegisterOmniboxKeyword(
-     const std::string& extension_id,
-     const std::string& extension_name,
-     const std::string& keyword,
-     const std::string& template_url_string) {
+    const std::string& extension_id,
+    const std::string& extension_name,
+    const std::string& keyword,
+    const std::string& template_url_string,
+    const base::Time& extension_install_time) {
   DCHECK(loaded_);
 
   if (FindTemplateURLForExtension(extension_id,
@@ -546,6 +547,7 @@ void TemplateURLService::RegisterOmniboxKeyword(
   data.SetURL(template_url_string);
   std::unique_ptr<TemplateURL::AssociatedExtensionInfo> info(
       new TemplateURL::AssociatedExtensionInfo(extension_id));
+  info->install_time = extension_install_time;
   AddExtensionControlledTURL(
       base::MakeUnique<TemplateURL>(data, TemplateURL::OMNIBOX_API_EXTENSION),
       std::move(info));
@@ -1443,6 +1445,39 @@ void TemplateURLService::Init(const Initializer* initializers,
   RequestGoogleURLTrackerServerCheckIfNecessary();
 }
 
+TemplateURL* TemplateURLService::BestEngineForKeyword(TemplateURL* engine1,
+                                                      TemplateURL* engine2) {
+  DCHECK(engine1 || engine2);
+  // If one is nullptr other is better alternative.
+  if (!engine1)
+    return engine2;
+  if (!engine2)
+    return engine1;
+
+  DCHECK_EQ(engine1->keyword(), engine2->keyword());
+  // We should only have overlapping keywords when at least one comes from
+  // an extension.
+  DCHECK(IsCreatedByExtension(engine1) || IsCreatedByExtension(engine2));
+
+  // Following ranking is implemented:
+  // omnibox api keyword > extension search engine > normal engine.
+  // If both extensions are of same type, latest installed wins.
+  if (engine2->type() == engine1->type())

[vasilii, 2017/02/21 13:17:51]

Add {}, same below for the multiline statements

[Alexander Yashkin, 2017/02/21 14:02:21]

Done.

+    return engine1->extension_info_->install_time >
+                   engine2->extension_info_->install_time
+               ? engine1
+               : engine2;
+  // Extension search engine loses to omnibox api keyword but wins on normal
+  // engine.
+  if (engine2->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)
+    return engine1->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine1
+                                                                 : engine2;
+  // Omnibox api keyword always wins. Normal engine loses to any extensions
+  // keywords.
+  return engine2->type() == TemplateURL::OMNIBOX_API_EXTENSION ? engine2
+                                                               : engine1;
+}
+
 void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
   const base::string16& keyword = template_url->keyword();
   DCHECK_NE(0U, keyword_to_turl_and_length_.count(keyword));
@@ -1455,14 +1490,8 @@ void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
     // highest ID).
     TemplateURL* best_fallback = nullptr;
     for (const auto& turl : template_urls_) {
-      // This next statement relies on the fact that there can only be one
-      // non-Omnibox API TemplateURL with a given keyword.
-      if ((turl.get() != template_url) && (turl->keyword() == keyword) &&
-          (!best_fallback ||
-           (best_fallback->type() != TemplateURL::OMNIBOX_API_EXTENSION) ||
-           ((turl->type() == TemplateURL::OMNIBOX_API_EXTENSION) &&
-            (turl->id() > best_fallback->id()))))
-        best_fallback = turl.get();
+      if ((turl.get() != template_url) && (turl->keyword() == keyword))
+        best_fallback = BestEngineForKeyword(best_fallback, turl.get());
     }
     RemoveFromDomainMap(template_url);
     if (best_fallback) {
@@ -1494,16 +1523,8 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) {
     AddToMap(template_url);
     AddToDomainMap(template_url);
   } else {
-    const TemplateURL* existing_url = i->second.first;
-    // We should only have overlapping keywords when at least one comes from
-    // an extension.  In that case, the ranking order is:
-    //   Manually-modified keywords > extension keywords > replaceable keywords
-    // When there are multiple extensions, the last-added wins.
-    bool existing_url_is_omnibox_api =
-        existing_url->type() == TemplateURL::OMNIBOX_API_EXTENSION;
-    DCHECK(existing_url_is_omnibox_api || template_url_is_omnibox_api);
-    if (existing_url_is_omnibox_api ?
-        !CanReplace(template_url) : CanReplace(existing_url)) {
+    TemplateURL* existing_url = i->second.first;
+    if (BestEngineForKeyword(existing_url, template_url) != existing_url) {
       RemoveFromDomainMap(existing_url);
       AddToMap(template_url);
       AddToDomainMap(template_url);
@@ -1651,7 +1672,7 @@ bool TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl,
   if (!Contains(&template_urls_, existing_turl))
     return false;
 
-  DCHECK_NE(TemplateURL::OMNIBOX_API_EXTENSION, existing_turl->type());
+  DCHECK(!IsCreatedByExtension(existing_turl));
 
   base::string16 old_keyword(existing_turl->keyword());
   keyword_to_turl_and_length_.erase(old_keyword);
@@ -1962,8 +1983,7 @@ bool TemplateURLService::ApplyDefaultSearchChangeNoMetrics(
   if (!data) {
     default_search_provider_ = nullptr;
   } else if (source == DefaultSearchManager::FROM_EXTENSION) {
-    default_search_provider_ = FindMatchingExtensionTemplateURL(
-        *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+    default_search_provider_ = FindMatchingDefaultExtensionTemplateURL(*data);
   } else if (source == DefaultSearchManager::FROM_FALLBACK) {
     default_search_provider_ =
         FindPrepopulatedTemplateURL(data->prepopulate_id);
@@ -2032,39 +2052,36 @@ 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 =
-      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());
-    // 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 {
-      base::string16 new_keyword =
-          UniquifyKeyword(*existing_keyword_turl, false);
-      ResetTemplateURLNoNotify(existing_keyword_turl,
-                               existing_keyword_turl->short_name(), new_keyword,
-                               existing_keyword_turl->url());
+
+  // Check for conflicts between non extension engines keywords.
+  if (!IsCreatedByExtension(template_url.get())) {
+    // Check whether |template_url|'s keyword conflicts with any already in the
+    // model.
+    TemplateURL* existing_turl =
+        FindNonExtensionTemplateURLForKeyword(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 (existing_turl && Contains(&template_urls_, existing_turl)) {
+      DCHECK_NE(existing_turl, template_url.get());
+      if (CanReplace(existing_turl)) {
+        RemoveNoNotify(existing_turl);
+      } else if (CanReplace(template_url.get())) {
+        return nullptr;
+      } else {
+        base::string16 new_keyword = UniquifyKeyword(*existing_turl, false);
+        // Resolve keyword conflicts for engines that can not be replaced.
+        // Replace keyword for existing engine.
+        ResetTemplateURLNoNotify(existing_turl, existing_turl->short_name(),
+                                 new_keyword, existing_turl->url());
+      }
     }
   }
   TemplateURL* template_url_ptr = template_url.get();
@@ -2208,6 +2225,7 @@ void TemplateURLService::ResetTemplateURLGUID(TemplateURL* url,
 
 base::string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
                                                    bool force) {
+  DCHECK(!IsCreatedByExtension(&turl));
   if (!force) {
     // Already unique.
     if (!GetTemplateURLForKeyword(turl.keyword()))
@@ -2216,8 +2234,7 @@ base::string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
     // First, try to return the generated keyword for the TemplateURL (except
     // for extensions, as their keywords are not associated with their URLs).
     GURL gurl(turl.url());
-    if (gurl.is_valid() &&
-        (turl.type() != TemplateURL::OMNIBOX_API_EXTENSION)) {
+    if (gurl.is_valid()) {
       base::string16 keyword_candidate = TemplateURL::GenerateKeyword(gurl);
       if (!GetTemplateURLForKeyword(keyword_candidate))
         return keyword_candidate;
@@ -2470,12 +2487,11 @@ TemplateURL* TemplateURLService::FindTemplateURLForExtension(
   return nullptr;
 }
 
-TemplateURL* TemplateURLService::FindMatchingExtensionTemplateURL(
-    const TemplateURLData& data,
-    TemplateURL::Type type) {
-  DCHECK_NE(TemplateURL::NORMAL, type);
+TemplateURL* TemplateURLService::FindMatchingDefaultExtensionTemplateURL(
+    const TemplateURLData& data) {
   for (const auto& turl : template_urls_) {
-    if (turl->type() == type &&
+    if (turl->type() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION &&
+        turl->extension_info_->wants_to_be_default_engine &&
         TemplateURL::MatchesData(turl.get(), &data, search_terms_data()))
       return turl.get();
   }