Changed keywords conflicts resolution for extensions search engines.

chrome/browser/search_engines/template_url_service_unittest.cc

Index: chrome/browser/search_engines/template_url_service_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc
index 07abd01787e417ddfe0c61ae831ce0db68cf4db4..be5af5cedfb827acb98801da02642508c9d5f95f 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -176,6 +176,12 @@ class TemplateURLServiceTest : public testing::Test {
                                   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);
 
@@ -253,6 +259,26 @@ TemplateURL* TemplateURLServiceTest::AddKeywordWithDate(
                               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());
@@ -470,33 +496,36 @@ TEST_F(TemplateURLServiceTest, AddExtensionKeyword) {
   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", base::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",
+                                  base::Time::FromDoubleT(2));
   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",
+                                  base::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",
+                                  base::Time::FromDoubleT(4));
   TemplateURL* extension3 = model()->FindTemplateURLForExtension(
       "id3", TemplateURL::OMNIBOX_API_EXTENSION);
   ASSERT_TRUE(extension3);
@@ -511,7 +540,7 @@ TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
   // replaceable TemplateURL.  We should still do correct conflict resolution
   // between the non-template URLs.
   model()->RegisterOmniboxKeyword("test2", "extension", "keyword",
-                                  "http://test2");
+                                  "http://test2", base::Time::Now());

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

If we don't care about the time I'd use Time(). Same in
NotPersistOmniboxExtensionKeyword.

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

Done.

   TemplateURL* extension =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"));
   ASSERT_TRUE(extension);
@@ -533,13 +562,13 @@ TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {
   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.
   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")));
 }
 
@@ -548,7 +577,7 @@ TEST_F(TemplateURLServiceTest, NotPersistOmniboxExtensionKeyword) {
 
   // Register an omnibox keyword.
   model()->RegisterOmniboxKeyword("test", "extension", "keyword",
-                                  "chrome-extension://test");
+                                  "chrome-extension://test", base::Time::Now());
   ASSERT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 
   // Reload the data.
@@ -879,7 +908,7 @@ TEST_F(TemplateURLServiceTest, RepairPrepopulatedSearchEngines) {
 
   // Register an extension with bing keyword.
   model()->RegisterOmniboxKeyword("abcdefg", "extension_name", "bing.com",
-                                  "http://abcdefg");
+                                  "http://abcdefg", base::Time::Now());
   EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("bing.com")));
 
   model()->RepairPrepopulatedSearchEngines();
@@ -1658,6 +1687,113 @@ TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
   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, base::Time::Now());

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

Again here we want to exclude the possibility that time matters when resolving a
conflict between different types.
I'd use an explicit timestamp here later than in #1720.

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

Done.

+
+  // 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.
+  model()->RegisterOmniboxKeyword("omnibox_api_extension_id", "extension_name",
+                                  "common_keyword", "http://test3",
+                                  base::Time::Now());
+  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.
+TEST_F(TemplateURLServiceTest, ExtensionsWithSameKeywords) {
+  test_util()->VerifyLoad();
+
+  // Add non default extension engine.
+  const TemplateURL* extension1 = AddExtensionSearchEngine(
+      "common_keyword", "extension_id1", false, base::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, base::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, base::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();