Make extensions DSE persistent in browser prefs (Reland)

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 ddc06380b068dc5a15b115a81b7277584f0d54a4..ca2100632af34a7020bde0a432a3f6b974395ae4 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -28,6 +28,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/search_engines/keyword_web_data_service.h"
+#include "components/search_engines/search_engines_test_util.h"
 #include "components/search_engines/search_host_to_urls_map.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
@@ -120,11 +121,15 @@ TemplateURL* AddKeywordWithDate(TemplateURLService* model,
 
 // Checks that the two TemplateURLs are similar. It does not check the id or
 // any time-related fields. Neither pointer should be NULL.
-void ExpectSimilar(const TemplateURL* expected, const TemplateURL* actual) {
+void ExpectSimilar(const TemplateURL* expected,
+                   const TemplateURL* actual,
+                   bool ignore_keyword = false) {
   ASSERT_TRUE(expected != NULL);
   ASSERT_TRUE(actual != NULL);
   EXPECT_EQ(expected->short_name(), actual->short_name());
-  EXPECT_EQ(expected->keyword(), actual->keyword());
+  if (!ignore_keyword) {

[Peter Kasting, 2017/01/25 22:31:23]

Nit: No {}

[Alexander Yashkin, 2017/01/26 07:51:01]

Done

+    EXPECT_EQ(expected->keyword(), actual->keyword());
+  }
   EXPECT_EQ(expected->url(), actual->url());
   EXPECT_EQ(expected->suggestions_url(), actual->suggestions_url());
   EXPECT_EQ(expected->favicon_url(), actual->favicon_url());
@@ -1434,16 +1439,15 @@ TEST_F(TemplateURLServiceTest, DefaultExtensionEngine) {
   std::unique_ptr<TemplateURL::AssociatedExtensionInfo> extension_info(
       new TemplateURL::AssociatedExtensionInfo("ext"));
   extension_info->wants_to_be_default_engine = true;
-  TemplateURL* ext_dse_ptr = model()->AddExtensionControlledTURL(
+  TemplateURL* ext_dse_ptr = test_util()->AddExtensionControlledTURL(
       std::move(ext_dse), std::move(extension_info));
   EXPECT_EQ(ext_dse_ptr, model()->GetDefaultSearchProvider());
 
-  model()->RemoveExtensionControlledTURL(
-      "ext", TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  test_util()->RemoveExtensionControlledTURL("ext");
   ExpectSimilar(user_dse, model()->GetDefaultSearchProvider());
 }
 
-TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
+TEST_F(TemplateURLServiceTest, DefaultExtensionEnginePersist) {
   test_util()->VerifyLoad();
   // Add third-party default search engine.
   TemplateURL* user_dse = AddKeywordWithDate(
@@ -1452,6 +1456,7 @@ TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
   model()->SetUserSelectedDefaultSearchProvider(user_dse);
   EXPECT_EQ(user_dse, model()->GetDefaultSearchProvider());
 
+  // Create non-default extension search engine.
   std::unique_ptr<TemplateURL> ext_dse = CreateKeywordWithDate(
       model(), "ext1", "ext1", "http://www.ext1.com/s?q={searchTerms}",
       std::string(), std::string(), std::string(), true, 0, "UTF-8", Time(),
@@ -1459,10 +1464,12 @@ TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
   std::unique_ptr<TemplateURL::AssociatedExtensionInfo> extension_info(
       new TemplateURL::AssociatedExtensionInfo("ext1"));
   extension_info->wants_to_be_default_engine = false;
-  model()->AddExtensionControlledTURL(std::move(ext_dse),
-                                      std::move(extension_info));
+
+  test_util()->AddExtensionControlledTURL(std::move(ext_dse),
+                                          std::move(extension_info));
   EXPECT_EQ(user_dse, model()->GetDefaultSearchProvider());
 
+  // Create default extension search engine.
   ext_dse = CreateKeywordWithDate(
       model(), "ext2", "ext2", "http://www.ext2.com/s?q={searchTerms}",
       std::string(), std::string(), std::string(), true, kPrepopulatedId,
@@ -1470,15 +1477,117 @@ TEST_F(TemplateURLServiceTest, ExtensionEnginesNotPersist) {
       TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
   extension_info.reset(new TemplateURL::AssociatedExtensionInfo("ext2"));
   extension_info->wants_to_be_default_engine = true;
-  TemplateURL* ext_dse_ptr = model()->AddExtensionControlledTURL(
+
+  auto cloned_ext_dse = base::MakeUnique<TemplateURL>(ext_dse->data());
+  TemplateURL* ext_dse_ptr = test_util()->AddExtensionControlledTURL(
       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());
+  // A default search engine set by an extension must be persisted across
+  // browser restarts, until the extension is unloaded/disabled.
+  test_util()->ResetModel(false);
+  EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext2")));
+  ExpectSimilar(cloned_ext_dse.get(), model()->GetDefaultSearchProvider());
+
+  // Non-default extension engines are not persisted across restarts.
   EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1")));
-  EXPECT_FALSE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext2")));
+}
+
+TEST_F(TemplateURLServiceTest, DefaultExtensionEnginePersistsBeforeLoad) {
+  // Chrome will load the extension system before the TemplateURLService, so
+  // extensions controlling the default search engine may be registered before
+  // the service has loaded.
+  std::unique_ptr<TemplateURL> ext_dse = CreateKeywordWithDate(
+      model(), "ext2", "ext2", "http://www.ext2.com/s?q={searchTerms}",
+      std::string(), std::string(), std::string(), true, kPrepopulatedId,
+      "UTF-8", Time(), Time(), Time(),
+      TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto extension_info =
+      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("ext2");
+  extension_info->wants_to_be_default_engine = true;
+
+  auto cloned_ext_dse = base::MakeUnique<TemplateURL>(ext_dse->data());
+  test_util()->AddExtensionControlledTURL(std::move(ext_dse),
+                                          std::move(extension_info));
+
+  // Default search engine from extension must be persisted between browser
+  // restarts, and should be available before the TemplateURLService is loaded.
+  EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext2")));
+  ExpectSimilar(cloned_ext_dse.get(), model()->GetDefaultSearchProvider());
+
+  // Check extension DSE is the same after service load.
+  test_util()->VerifyLoad();
+  ExpectSimilar(cloned_ext_dse.get(), model()->GetDefaultSearchProvider());
+}
+
+// Check that extension engine and prepopulated engine with same keywords are
+// handled corrected.
+TEST_F(TemplateURLServiceTest, DefaultExtensionEngineAndPrepopulated) {
+  // Add default extension engine.
+  auto ext_data = GenerateDummyTemplateURLData("ext2");
+  ext_data->safe_for_autoreplace = false;
+  auto ext_dse = base::MakeUnique<TemplateURL>(
+      *ext_data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto extension_info =
+      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("ext2");
+  extension_info->wants_to_be_default_engine = true;
+
+  auto cloned_ext_dse = base::MakeUnique<TemplateURL>(ext_dse->data());
+  test_util()->AddExtensionControlledTURL(std::move(ext_dse),
+                                          std::move(extension_info));
+
+  // Add prepopulated engine with same keyword as extension.
+  model()->Add(
+      base::MakeUnique<TemplateURL>(*GenerateDummyTemplateURLData("ext2")));
+  // Wait for any saves to finish.
+  base::RunLoop().RunUntilIdle();
+
+  test_util()->VerifyLoad();
+  // Check extension DSE is set as default. Due to keywords conflict with
+  // prepopulated engine its keyword changed.
+  EXPECT_TRUE(model()->IsExtensionControlledDefaultSearch());
+  ExpectSimilar(cloned_ext_dse.get(), model()->GetDefaultSearchProvider(),
+                true);

[Peter Kasting, 2017/01/25 22:31:23]

Should we be checking as to which engine's keyword got changed?

[Alexander Yashkin, 2017/01/26 07:51:01]

Done

+}
+
+// Check that two extensions with same engine handled corrected.

[Peter Kasting, 2017/01/25 22:31:23]

What parts of the engines being "the same" are relevant here?  The keyword?  The
search URL?  The fact that literally every piece of data from the two engines is
the same?

[Alexander Yashkin, 2017/01/26 07:51:01]

The keyword conflict is relevant. Changed comments and code to highlight this.

+TEST_F(TemplateURLServiceTest, TwoExtensionsWithSameEngine) {
+  // Add default extension engine.
+  auto ext_data = GenerateDummyTemplateURLData("extension");
+  ext_data->safe_for_autoreplace = false;
+  auto ext_dse = base::MakeUnique<TemplateURL>(
+      *ext_data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto extension_info =
+      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("extension_id1");
+  extension_info->wants_to_be_default_engine = true;
+
+  auto cloned_ext_dse = base::MakeUnique<TemplateURL>(ext_dse->data());
+  test_util()->AddExtensionControlledTURL(std::move(ext_dse),
+                                          std::move(extension_info));
+
+  // Add non default extension with same engine. This action will change
+  // previous extension keyword due to conflict.
+  ext_data = GenerateDummyTemplateURLData("extension");
+  ext_data->safe_for_autoreplace = false;
+  ext_dse = base::MakeUnique<TemplateURL>(
+      *ext_data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  extension_info =
+      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("extension_id2");
+  extension_info->wants_to_be_default_engine = false;
+
+  test_util()->AddExtensionControlledTURL(std::move(ext_dse),
+                                          std::move(extension_info));
+  // Wait for any saves to finish.
+  base::RunLoop().RunUntilIdle();
+
+  test_util()->VerifyLoad();
+  // Check that extension DSE is set as default. Due to keywords conflict with
+  // prepopulated engine its keyword changed.
+  auto current_dse = model()->GetDefaultSearchProvider();
+  EXPECT_TRUE(model()->IsExtensionControlledDefaultSearch());
+  ExpectSimilar(cloned_ext_dse.get(), current_dse, true);
+  EXPECT_EQ("extension_id1",
+            current_dse->GetExtensionInfoForTesting()->extension_id);
 }
 
 TEST_F(TemplateURLServiceTest, ExtensionEngineVsPolicy) {
@@ -1501,7 +1610,7 @@ TEST_F(TemplateURLServiceTest, ExtensionEngineVsPolicy) {
   auto extension_info =
       base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("ext1");
   extension_info->wants_to_be_default_engine = true;
-  TemplateURL* ext_dse_ptr = model()->AddExtensionControlledTURL(
+  TemplateURL* ext_dse_ptr = test_util()->AddExtensionControlledTURL(
       std::move(ext_dse), std::move(extension_info));
   EXPECT_EQ(ext_dse_ptr,
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("ext1")));