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..4359046f6639ccd026f581794e1e2d6348e3161a 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,41 @@ void TemplateURLService::Init(const Initializer* initializers,
   RequestGoogleURLTrackerServerCheckIfNecessary();
 }
 
+TemplateURL* TemplateURLService::BestEngineForKeyword(
+    TemplateURL* existing_engine,
+    TemplateURL* new_engine) {
+  DCHECK(new_engine);
+  if (!existing_engine)
+    return new_engine;  // Better than nothing.
+
+  DCHECK_EQ(new_engine->keyword(), existing_engine->keyword());
+  // We should only have overlapping keywords when at least one comes from
+  // an extension.
+  DCHECK(IsCreatedByExtension(existing_engine) ||
+         IsCreatedByExtension(new_engine));
+
+  // Following ranking is implemented:
+  // omnibox api keyword > extension search engine > normal engine.
+  // If both extensions are of same type, latest installed wins.
+  if (new_engine->type() == existing_engine->type()) {
+    return existing_engine->extension_info_->install_time >
+                   new_engine->extension_info_->install_time
+               ? existing_engine
+               : new_engine;
+  } else if (new_engine->type() ==

[vasilii, 2017/02/20 15:30:08]

You don't need else as the previous block always returns something.

[Alexander Yashkin, 2017/02/20 19:52:47]

Done.

+             TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) {
+    return existing_engine->type() == TemplateURL::OMNIBOX_API_EXTENSION
+               ? existing_engine
+               : new_engine;
+  } else {
+    // Omnibox api keyword always wins. Normal engine loses to any extensions
+    // keywords.
+    return new_engine->type() == TemplateURL::OMNIBOX_API_EXTENSION
+               ? new_engine
+               : existing_engine;
+  }
+}
+
 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 +1492,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 +1525,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 +1674,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 +1985,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 +2054,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))

[vasilii, 2017/02/20 15:30:08]

{} here and below.

[Alexander Yashkin, 2017/02/20 19:52:47]

Done.

+        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 +2227,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 +2236,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 +2489,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();
   }