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

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

I'd rename the test to AddOmniboxExtensionKeyword

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

If you do not address a comment you should explain why.

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

Sorry missed it. Where is a bug in codereview tool I think.

I see this comment only if I turn on "Deprecated UI" setting in my account
settings and switch to "View side-by-side diff with in-line comments" mode.

If I turn off "Deprecated UI" or switch to "View unified diff" mode this comment
and another one below with "AddSameKeywordWithOmniboxExtensionPresent" text is
not shown at all. 
Thats very embarassing.

Test name fixed.

   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", 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);
   EXPECT_EQ(extension3,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword3")));
 }
 
 TEST_F(TemplateURLServiceTest, AddSameKeywordWithExtensionPresent) {

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

AddSameKeywordWithOmniboxExtensionPresent

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

Done.

   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", base::Time::Now());
   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.
   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", base::Time::Now());
   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", base::Time::Now());
   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")));
 }
 
+// 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();
   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);
 }