Changed keywords conflicts resolution for extensions search engines.

chrome/browser/search_engines/template_url_service_unittest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
 #include <memory>
 #include <utility>
@@ skipping 158 matching lines @@
                                   const std::string& url,
                                   const std::string& suggest_url,
                                   const std::string& alternate_url,
                                   const std::string& favicon_url,
                                   bool safe_for_autoreplace,
                                   const std::string& encodings,
                                   Time date_created,
                                   Time last_modified,
                                   Time last_visited);
 
+  // Add extension controlled search engine with |keyword| to model.
+  TemplateURL* AddExtensionSearchEngine(const std::string& keyword,
+                                        const std::string& extension_name,
+                                        bool wants_to_be_default_engine,
+                                        const base::Time& install_time);
+
   // Verifies the two TemplateURLs are equal.
   void AssertEquals(const TemplateURL& expected, const TemplateURL& actual);
 
   // Verifies the two timestamps are equal, within the expected degree of
   // precision.
   void AssertTimesEqual(const base::Time& expected, const base::Time& actual);
 
   // Create an URL that appears to have been prepopulated, but won't be in the
   // current data.
   std::unique_ptr<TemplateURL> CreatePreloadedTemplateURL(
@@ skipping 57 matching lines @@
     const std::string& encodings,
     Time date_created,
     Time last_modified,
     Time last_visited) {
   return ::AddKeywordWithDate(model(), short_name, keyword, url, suggest_url,
                               alternate_url, favicon_url, safe_for_autoreplace,
                               encodings, date_created, last_modified,
                               last_visited);
 }
 
+TemplateURL* TemplateURLServiceTest::AddExtensionSearchEngine(
+    const std::string& keyword,
+    const std::string& extension_name,
+    bool wants_to_be_default_engine,
+    const base::Time& install_time) {
+  std::unique_ptr<TemplateURLData> turl_data =
+      GenerateDummyTemplateURLData(keyword);
+  turl_data->safe_for_autoreplace = false;
+
+  auto ext_dse = base::MakeUnique<TemplateURL>(
+      *turl_data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto extension_info =
+      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>(extension_name);
+  extension_info->wants_to_be_default_engine = wants_to_be_default_engine;
+  extension_info->install_time = install_time;
+
+  return model()->AddExtensionControlledTURL(std::move(ext_dse),
+                                             std::move(extension_info));
+}
+
 void TemplateURLServiceTest::AssertEquals(const TemplateURL& expected,
                                           const TemplateURL& actual) {
   ASSERT_EQ(expected.short_name(), actual.short_name());
   ASSERT_EQ(expected.keyword(), actual.keyword());
   ASSERT_EQ(expected.url(), actual.url());
   ASSERT_EQ(expected.suggestions_url(), actual.suggestions_url());
   ASSERT_EQ(expected.favicon_url(), actual.favicon_url());
   ASSERT_EQ(expected.alternate_urls(), actual.alternate_urls());
   ASSERT_EQ(expected.prepopulate_id(), actual.prepopulate_id());
   ASSERT_EQ(expected.safe_for_autoreplace(), actual.safe_for_autoreplace());
@@ skipping 191 matching lines @@
   data.safe_for_autoreplace = false;
   TemplateURL* t_url2 = model()->Add(base::MakeUnique<TemplateURL>(data));
   VerifyObserverCount(1);
   EXPECT_EQ(t_url2, model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
   EXPECT_EQ(ASCIIToUTF16("fourth"), t_url2->short_name());
   EXPECT_EQ(ASCIIToUTF16("keyword"), t_url2->keyword());
   EXPECT_EQ(ASCIIToUTF16("second"), t_url->short_name());
   EXPECT_EQ(ASCIIToUTF16("test2"), t_url->keyword());
 }
 
-TEST_F(TemplateURLServiceTest, AddExtensionKeyword) {
+TEST_F(TemplateURLServiceTest, AddOmniboxExtensionKeyword) {
   test_util()->VerifyLoad();
 
   AddKeywordWithDate("replaceable", "keyword1", "http://test1", std::string(),
                      std::string(), std::string(), true, "UTF-8", Time(),
                      Time(), Time());
-  TemplateURL* original2 = AddKeywordWithDate(
-      "nonreplaceable", "keyword2", "http://test2", std::string(),
-      std::string(), std::string(), false, "UTF-8", Time(), Time(), Time());
+  AddKeywordWithDate("nonreplaceable", "keyword2", "http://test2",
+                     std::string(), std::string(), std::string(), false,
+                     "UTF-8", Time(), Time(), Time());
   model()->RegisterOmniboxKeyword("test3", "extension", "keyword3",
-                                  "http://test3");
+                                  "http://test3", Time::FromDoubleT(1));
   TemplateURL* original3 =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3"));
   ASSERT_TRUE(original3);
 
   // Extension keywords should override replaceable keywords.
-  model()->RegisterOmniboxKeyword("id1", "test", "keyword1", "http://test4");
+  model()->RegisterOmniboxKeyword("id1", "test", "keyword1", "http://test4",
+                                  Time::FromDoubleT(2));

[Peter Kasting, 2017/02/25 04:26:53]

Nit: Will anything crash if we just pass Time() in this case and the next one? 
That avoids implying that these time values matter.

[Alexander Yashkin, 2017/02/25 11:34:06]

Removed FromDoubleT in this two cases, time value is relevant only when
comparing two omnibox extensions keywords, 
in this test these are engines with keyword "keyword3".

   TemplateURL* extension1 = model()->FindTemplateURLForExtension(
       "id1", TemplateURL::OMNIBOX_API_EXTENSION);
   EXPECT_TRUE(extension1);
   EXPECT_EQ(extension1,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword1")));
 
-  // They should not override non-replaceable keywords.
-  model()->RegisterOmniboxKeyword("id2", "test", "keyword2", "http://test5");
+  // They should also override non-replaceable keywords.
+  model()->RegisterOmniboxKeyword("id2", "test", "keyword2", "http://test5",
+                                  Time::FromDoubleT(3));
   TemplateURL* extension2 = model()->FindTemplateURLForExtension(
       "id2", TemplateURL::OMNIBOX_API_EXTENSION);
   ASSERT_TRUE(extension2);
-  EXPECT_EQ(original2,
+  EXPECT_EQ(extension2,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword2")));
 
   // They should override extension keywords added earlier.
-  model()->RegisterOmniboxKeyword("id3", "test", "keyword3", "http://test6");
+  model()->RegisterOmniboxKeyword("id3", "test", "keyword3", "http://test6",
+                                  Time::FromDoubleT(4));
   TemplateURL* extension3 = model()->FindTemplateURLForExtension(
       "id3", TemplateURL::OMNIBOX_API_EXTENSION);
   ASSERT_TRUE(extension3);
   EXPECT_EQ(extension3,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3")));
 }
 
-TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
+TEST_F(TemplateURLServiceTest, AddSameKeywordWithOmniboxExtensionPresent) {
   test_util()->VerifyLoad();
 
   // Similar to the AddSameKeyword test, but with an extension keyword masking a
   // replaceable TemplateURL.  We should still do correct conflict resolution
   // between the non-template URLs.
   model()->RegisterOmniboxKeyword("test2", "extension", "keyword",
-                                  "http://test2");
+                                  "http://test2", Time());
   TemplateURL* extension =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"));
   ASSERT_TRUE(extension);
   // Adding a keyword that matches the extension.
   AddKeywordWithDate("replaceable", "keyword", "http://test1", std::string(),
                      std::string(), std::string(), true, "UTF-8", Time(),
                      Time(), Time());
 
   // Adding another replaceable keyword should remove the existing one, but
   // leave the extension as is.
   TemplateURLData data;
   data.SetShortName(ASCIIToUTF16("name1"));
   data.SetKeyword(ASCIIToUTF16("keyword"));
   data.SetURL("http://test3");
   data.safe_for_autoreplace = true;
   TemplateURL* t_url = model()->Add(base::MakeUnique<TemplateURL>(data));
   EXPECT_EQ(extension,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
   EXPECT_EQ(t_url, model()->GetTemplateURLForHost("test3"));
 
   // Adding a nonreplaceable keyword should remove the existing replaceable
-  // keyword and replace the extension as the associated URL for this keyword,
-  // but not evict the extension from the service entirely.
+  // keyword, yet extension must still be set as the associated URL for this
+  // keyword.

[Peter Kasting, 2017/02/25 04:26:53]

It seems like the test no longer checks that the replaceable keyword has been
removed (underneath the extension).  We should probably use
GetTemplateURLForGUID() or GetTemplateURLForHost() to check that as well.

[Alexander Yashkin, 2017/02/25 11:34:06]

Done.

   data.SetShortName(ASCIIToUTF16("name2"));
   data.SetURL("http://test4");
   data.safe_for_autoreplace = false;
-  TemplateURL* t_url2 = model()->Add(base::MakeUnique<TemplateURL>(data));
-  EXPECT_EQ(t_url2,
+  model()->Add(base::MakeUnique<TemplateURL>(data));
+  EXPECT_EQ(extension,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 }
 
 TEST_F(TemplateURLServiceTest, NotPersistOmniboxExtensionKeyword) {
   test_util()->VerifyLoad();
 
   // Register an omnibox keyword.
   model()->RegisterOmniboxKeyword("test", "extension", "keyword",
-                                  "chrome-extension://test");
+                                  "chrome-extension://test", Time());
   ASSERT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 
   // Reload the data.
   test_util()->ResetModel(true);
 
   // Ensure the omnibox keyword is not persisted.
   ASSERT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 }
 
 TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) {
@@ skipping 310 matching lines @@
 
   // Remove bing.
   TemplateURL* bing = model()->GetTemplateURLForKeyword(
       ASCIIToUTF16("bing.com"));
   ASSERT_TRUE(bing);
   model()->Remove(bing);
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("bing.com")));
 
   // Register an extension with bing keyword.
   model()->RegisterOmniboxKeyword("abcdefg", "extension_name", "bing.com",
-                                  "http://abcdefg");
+                                  "http://abcdefg", Time());
   EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("bing.com")));
 
   model()->RepairPrepopulatedSearchEngines();
 
   // Google is default.
   ASSERT_EQ(google, model()->GetDefaultSearchProvider());
   // The keyword wasn't reverted.
   EXPECT_EQ(ASCIIToUTF16("trash"), google->short_name());
   EXPECT_EQ("www.google.com",
             google->GenerateSearchURL(model()->search_terms_data()).host());
@@ skipping 758 matching lines @@
       std::move(ext_dse), std::move(extension_info));
   EXPECT_EQ(ext_dse_ptr, model()->GetDefaultSearchProvider());
 
   test_util()->ResetModel(true);
   user_dse = model()->GetTemplateURLForKeyword(ASCIIToUTF16("user"));
   ExpectSimilar(user_dse, model()->GetDefaultSearchProvider());
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1")));
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext2")));
 }
 
+// Checks that correct priority is applied when resolving conflicts between the
+// omnibox extension, search engine extension and user search engines with same
+// keyword.
+TEST_F(TemplateURLServiceTest, CheckEnginesWithSameKeywords) {
+  test_util()->VerifyLoad();
+  // TemplateURLData used for user engines.
+  std::unique_ptr<TemplateURLData> turl_data =
+      GenerateDummyTemplateURLData("common_keyword");
+  turl_data->safe_for_autoreplace = false;
+
+  // Add non replaceable user engine.
+  const TemplateURL* user1 =
+      model()->Add(base::MakeUnique<TemplateURL>(*turl_data));
+
+  // Add default extension engine with same keyword as user engine.
+  const TemplateURL* extension = AddExtensionSearchEngine(
+      "common_keyword", "extension_id", true, Time::FromDoubleT(2));
+
+  // Add another non replaceable user engine with same keyword as extension.
+  const TemplateURL* user2 =
+      model()->Add(base::MakeUnique<TemplateURL>(*turl_data));
+
+  // Check extension DSE is set as default and its keyword is not changed.
+  auto current_dse = model()->GetDefaultSearchProvider();
+  EXPECT_EQ(extension, current_dse);
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), current_dse->keyword());
+
+  // Register omnibox keyword with same keyword as extension.
+  // Use |install_time| value less than in AddExtensionSearchEngine call above
+  // to check that omnibox api keyword is ranked higher even if installed
+  // earlier.
+  model()->RegisterOmniboxKeyword("omnibox_api_extension_id", "extension_name",
+                                  "common_keyword", "http://test3",
+                                  Time::FromDoubleT(1));
+  TemplateURL* omnibox_api = model()->FindTemplateURLForExtension(
+      "omnibox_api_extension_id", TemplateURL::OMNIBOX_API_EXTENSION);
+
+  // Expect that first non replaceable user engine keyword is changed because of
+  // conflict. Second user engine will keep its keyword.
+  EXPECT_NE(ASCIIToUTF16("common_keyword"), user1->keyword());
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), user2->keyword());
+
+  // Check that extensions kept their keywords.
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), extension->keyword());
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), omnibox_api->keyword());
+
+  // Omnibox api is accessible by keyword as most relevant.
+  EXPECT_EQ(omnibox_api,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+  // Extension controlled search engine is still set as default and can be found
+  // in TemplateURLService.
+  EXPECT_EQ(extension, model()->GetDefaultSearchProvider());
+  EXPECT_EQ(extension,
+            model()->FindTemplateURLForExtension(
+                "extension_id", TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION));
+
+  // Test removing engines.
+  // Remove omnibox api extension.
+  model()->RemoveExtensionControlledTURL("omnibox_api_extension_id",
+                                         TemplateURL::OMNIBOX_API_EXTENSION);
+  // Expect that keyword is now corresponds to extension search engine.
+  EXPECT_EQ(extension,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+  // Remove extension engine.
+  model()->RemoveExtensionControlledTURL(
+      "extension_id", TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  EXPECT_NE(extension, model()->GetDefaultSearchProvider());
+  // Now latest user engine is returned for keyword.
+  EXPECT_EQ(user2,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+}
+
+// Check that two extensions with same engine are handled corrected.

[Peter Kasting, 2017/02/25 04:26:53]

Nit: same engine -> the same keyword; corrected -> correctly

[Alexander Yashkin, 2017/02/25 11:34:06]

Done, thanks.

+TEST_F(TemplateURLServiceTest, ExtensionsWithSameKeywords) {
+  test_util()->VerifyLoad();
+
+  // Add non default extension engine.
+  const TemplateURL* extension1 = AddExtensionSearchEngine(
+      "common_keyword", "extension_id1", false, Time::FromDoubleT(1));
+
+  // Check that GetTemplateURLForKeyword returns last installed extension.
+  EXPECT_EQ(extension1,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+
+  // Add default extension engine with the same keyword.
+  const TemplateURL* extension2 = AddExtensionSearchEngine(
+      "common_keyword", "extension_id2", true, Time::FromDoubleT(2));
+
+  // Check that GetTemplateURLForKeyword now returns extension2 because it was
+  // installed later.
+  EXPECT_EQ(extension2,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+
+  // Add another non default extension with same keyword. This action must not
+  // change any keyword due to conflict.
+  const TemplateURL* extension3 = AddExtensionSearchEngine(
+      "common_keyword", "extension_id3", false, Time::FromDoubleT(3));
+
+  // Check that extension2 is set as default.
+  EXPECT_EQ(extension2, model()->GetDefaultSearchProvider());
+
+  // Check that GetTemplateURLForKeyword returns last installed extension.
+  EXPECT_EQ(extension3,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+  // Check that all keywords for extensions are left unchanged.
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), extension1->keyword());
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), extension2->keyword());
+  EXPECT_EQ(ASCIIToUTF16("common_keyword"), extension3->keyword());
+}
+
 TEST_F(TemplateURLServiceTest, ExtensionEngineVsPolicy) {
   // Set a managed preference that establishes a default search provider.
   std::unique_ptr<TemplateURLData> managed = CreateTestSearchEngine();
   SetManagedDefaultSearchPreferences(*managed, true, test_util()->profile());
   test_util()->VerifyLoad();
   // Verify that the default manager we are getting is the managed one.
   auto expected_managed_default = base::MakeUnique<TemplateURL>(*managed);
   EXPECT_TRUE(model()->is_default_search_managed());
   const TemplateURL* actual_managed_default =
       model()->GetDefaultSearchProvider();
@@ skipping 48 matching lines @@
   TemplateURL* update_url =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));
   const base::Time update_last_modified = update_url->last_modified();
   model()->SetUserSelectedDefaultSearchProvider(update_url);
   TemplateURL* reloaded_url =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));
   const base::Time reloaded_last_modified = reloaded_url->last_modified();
   EXPECT_NE(original_last_modified, reloaded_last_modified);
   EXPECT_EQ(update_last_modified, reloaded_last_modified);
 }