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..8fc55149ff28a45d6c0a5cb83ec3d417dace6d0d 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -176,6 +176,10 @@ class TemplateURLServiceTest : public testing::Test {
                                   Time last_modified,
                                   Time last_visited);
 
+  TemplateURL* AddExtensionSearchEngine(const std::string& keyword,

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

A comment?

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

Done.

+                                        const std::string& extension_name,
+                                        bool wants_to_be_default_engine);
+
   // Verifies the two TemplateURLs are equal.
   void AssertEquals(const TemplateURL& expected, const TemplateURL& actual);
 
@@ -253,6 +257,24 @@ TemplateURL* TemplateURLServiceTest::AddKeywordWithDate(
                               last_visited);
 }
 
+TemplateURL* TemplateURLServiceTest::AddExtensionSearchEngine(
+    const std::string& keyword,
+    const std::string& extension_name,
+    bool wants_to_be_default_engine) {
+  auto turl_data = GenerateDummyTemplateURLData(keyword);

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

The explicit type improves readabilty here.

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

Done.

+  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 = base::Time::Now();
+
+  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 +492,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::Now());
   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::Now());
   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::Now());
   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::Now());

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

I see a source of flakiness here. base::Time::Now() may return the same as in
#499 and then the outcome isn't clear.

I'd prefer to be explicit. Create a local variable with base::Time::Now() in the
begin of the test. Later use |time + FromDays(1)| where it's important.

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

Agree, changed to base::Time::FromDoubleT() in cases where time is compared.

   TemplateURL* extension3 = model()->FindTemplateURLForExtension(
       "id3", TemplateURL::OMNIBOX_API_EXTENSION);
   ASSERT_TRUE(extension3);
@@ -511,7 +536,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());
   TemplateURL* extension =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword"));
   ASSERT_TRUE(extension);
@@ -533,13 +558,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 +573,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 +904,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 +1683,117 @@ TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext2")));
 }
 
+// Check that default extension engine and user engines with same keywords are

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

It checks the priority between the omnibox extension, search engine extension
and user search engines.

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

Reworded.

+// handled corrected.
+TEST_F(TemplateURLServiceTest, DefaultExtensionAndEnginesWithSameKeywords) {
+  test_util()->VerifyLoad();
+  // TemplateURLData used for user engines.
+  auto 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);
+
+  // Add another non replaceable user engine with same keyword as extension.
+  const TemplateURL* user2 =
+      model()->Add(base::MakeUnique<TemplateURL>(*turl_data));
+
+  // Wait for any saves to finish.
+  base::RunLoop().RunUntilIdle();

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

Why?

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

Copied from another test. 
I think its unnecessary, so removed this call.

+
+  // 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_name",

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

Can you use "extension_id" or similar as the first parameter?

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

Changed to "omnibox_api_extension_id". 
Did I understood you right?

+                                  "common_keyword", "http://test3",
+                                  base::Time::Now());
+  TemplateURL* omnibox_api = model()->FindTemplateURLForExtension(
+      "omnibox_api", 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",
+                                         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);
+
+  // 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);

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

I think here is the same flakiness as above regarding the time.

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

Changed, now pass explicit time to AddExtensionSearchEngine.

+
+  // 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);
+
+  // Wait for any saves to finish.
+  base::RunLoop().RunUntilIdle();

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

Why?

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

Removed.

+
+  // 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();