Make extensions DSE persistent in browser prefs

components/search_engines/default_search_manager_unittest.cc

Index: components/search_engines/default_search_manager_unittest.cc
diff --git a/components/search_engines/default_search_manager_unittest.cc b/components/search_engines/default_search_manager_unittest.cc
index a8611d775e2bb715b93ac00bce9269858992aa4b..69e6760e0fe9e524261a06ce04e17ce3573b95fa 100644
--- a/components/search_engines/default_search_manager_unittest.cc
+++ b/components/search_engines/default_search_manager_unittest.cc
@@ -16,6 +16,7 @@
 #include "base/time/time.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/search_engines/search_engines_pref_names.h"
+#include "components/search_engines/search_engines_test_util.h"
 #include "components/search_engines/template_url_data.h"
 #include "components/search_engines/template_url_data_util.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
@@ -29,25 +30,6 @@ namespace {
 const char kDefaultSearchProviderData[] =
     "default_search_provider_data.template_url_data";

[Peter Kasting, 2016/12/22 20:49:04]

Seems like this could go away and be replaced by
DefaultSearchManager::kDefaultSearchProviderDataPrefName, which has the same
value and is in this same component.

[Alexander Yashkin, 2016/12/23 19:44:09]

Done. 
I thought about it, yet decided to leave until later refactoring, to keep scope
of current CL under control 
:)

 
-// Checks that the two TemplateURLs are similar. Does not check the id or
-// any time-related fields.  Neither pointer should be null.
-void ExpectSimilar(const TemplateURLData* expected,
-                   const TemplateURLData* actual) {
-  ASSERT_TRUE(expected != NULL);
-  ASSERT_TRUE(actual != NULL);
-
-  EXPECT_EQ(expected->short_name(), actual->short_name());
-  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);
-  EXPECT_EQ(expected->alternate_urls, actual->alternate_urls);
-  EXPECT_EQ(expected->safe_for_autoreplace, actual->safe_for_autoreplace);
-  EXPECT_EQ(expected->input_encodings, actual->input_encodings);
-  EXPECT_EQ(expected->search_terms_replacement_key,
-            actual->search_terms_replacement_key);
-}
-
 // TODO(caitkp): TemplateURLData-ify this.
 void SetOverrides(sync_preferences::TestingPrefServiceSyncable* prefs,
                   bool update) {
@@ -97,23 +79,12 @@ void SetPolicy(sync_preferences::TestingPrefServiceSyncable* prefs,
   prefs->SetManagedPref(kDefaultSearchProviderData, entry.release());
 }
 
-std::unique_ptr<TemplateURLData> GenerateDummyTemplateURLData(
-    const std::string& type) {
-  std::unique_ptr<TemplateURLData> data(new TemplateURLData());
-  data->SetShortName(base::UTF8ToUTF16(std::string(type).append("name")));
-  data->SetKeyword(base::UTF8ToUTF16(std::string(type).append("key")));
-  data->SetURL(std::string("http://").append(type).append("foo/{searchTerms}"));
-  data->suggestions_url = std::string("http://").append(type).append("sugg");
-  data->alternate_urls.push_back(
-      std::string("http://").append(type).append("foo/alt"));
-  data->favicon_url = GURL("http://icon1");
-  data->safe_for_autoreplace = true;
-  data->input_encodings = base::SplitString(
-      "UTF-8;UTF-16", ";", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-  data->date_created = base::Time();
-  data->last_modified = base::Time();
-  data->last_visited = base::Time();
-  return data;
+void SetExtensionControlledDefaultSearchEngine(
+    sync_preferences::TestingPrefServiceSyncable* prefs,
+    const TemplateURLData& data) {

[Peter Kasting, 2016/12/22 20:49:04]

Once the pref change I suggested above is made, this function will be very
similar to TemplateURLServiceTestUtil::SetExtensionDefaultSearchInPrefs(), which
you're adding.

Maybe it makes sense to just eliminate that function and its partner (or all
four functions there?) in favor of similar ones here in
components/search_engines/.

[Alexander Yashkin, 2016/12/23 19:44:09]

I have extracted SetExtensionDefaultSearchInPrefs and
RemoveExtensionDefaultSearchFromPrefs
to search_engines/search_engines_test_util and reused in both places.

Yet I am not sure about other functions, do you mean
AddExtensionControlledTURL/RemoveExtensionControlledTURL in
TemplateURLServiceTestUtil?
They must have access to model() and profile() in TemplateURLServiceTestUtil. 
And they look more like part of interface in TemplateURLServiceTestUtil.

+  std::unique_ptr<base::DictionaryValue> entry(

[Peter Kasting, 2016/12/22 20:49:03]

Nit: Use =

[Alexander Yashkin, 2016/12/23 19:44:08]

Done

+      TemplateURLDataToDictionary(data));
+  prefs->SetExtensionPref(kDefaultSearchProviderData, entry.release());
 }
 
 }  // namespace
@@ -265,43 +236,34 @@ TEST_F(DefaultSearchManagerTest, DefaultSearchSetByExtension) {
                                DefaultSearchManager::ObserverCallback());
   std::unique_ptr<TemplateURLData> data = GenerateDummyTemplateURLData("user");
   manager.SetUserSelectedDefaultSearchEngine(*data);
-

[Peter Kasting, 2016/12/22 20:49:04]

Nit: I wouldn't remove the blank lines in this test, I think they improved
readability.

[Alexander Yashkin, 2016/12/23 19:44:08]

I dont remember removing them intentionally, maybe it was git cl format.
Reverted

   DefaultSearchManager::Source source = DefaultSearchManager::FROM_FALLBACK;
   ExpectSimilar(data.get(), manager.GetDefaultSearchEngine(&source));
   EXPECT_EQ(DefaultSearchManager::FROM_USER, source);
-
   // Extension trumps prefs:
   std::unique_ptr<TemplateURLData> extension_data_1 =
       GenerateDummyTemplateURLData("ext1");
-  manager.SetExtensionControlledDefaultSearchEngine(*extension_data_1);
-
+  SetExtensionControlledDefaultSearchEngine(pref_service(), *extension_data_1);
   ExpectSimilar(extension_data_1.get(),
                 manager.GetDefaultSearchEngine(&source));
   EXPECT_EQ(DefaultSearchManager::FROM_EXTENSION, source);
-
   // Policy trumps extension:
   std::unique_ptr<TemplateURLData> policy_data =
       GenerateDummyTemplateURLData("policy");
   SetPolicy(pref_service(), true, policy_data.get());
-
   ExpectSimilar(policy_data.get(), manager.GetDefaultSearchEngine(&source));
   EXPECT_EQ(DefaultSearchManager::FROM_POLICY, source);
   pref_service()->RemoveManagedPref(kDefaultSearchProviderData);
-
   // Extensions trump each other:
   std::unique_ptr<TemplateURLData> extension_data_2 =
       GenerateDummyTemplateURLData("ext2");
   std::unique_ptr<TemplateURLData> extension_data_3 =
       GenerateDummyTemplateURLData("ext3");
-  manager.SetExtensionControlledDefaultSearchEngine(*extension_data_2);
-  manager.SetExtensionControlledDefaultSearchEngine(*extension_data_3);
-
+  SetExtensionControlledDefaultSearchEngine(pref_service(), *extension_data_2);
+  SetExtensionControlledDefaultSearchEngine(pref_service(), *extension_data_3);
   ExpectSimilar(extension_data_3.get(),
                 manager.GetDefaultSearchEngine(&source));
   EXPECT_EQ(DefaultSearchManager::FROM_EXTENSION, source);
-
-  manager.ClearExtensionControlledDefaultSearchEngine();
-
+  pref_service()->RemoveExtensionPref(kDefaultSearchProviderData);
   ExpectSimilar(data.get(), manager.GetDefaultSearchEngine(&source));
   EXPECT_EQ(DefaultSearchManager::FROM_USER, source);
 }