Make extensions DSE persistent in browser prefs

chrome/browser/search_engines/template_url_service_sync_unittest.cc

Index: chrome/browser/search_engines/template_url_service_sync_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_service_sync_unittest.cc b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
index 78f74c7843d187baef69723203939c6f2a547447..564bf1617a16637a9361ef8afa84550f6c632d38 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/search_engines/template_url_service_test_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/search_engines/search_engines_pref_names.h"
+#include "components/search_engines/search_engines_test_util.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
@@ -413,7 +414,7 @@ TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataBasic) {
   }
 }
 
-TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataWithExtension) {
+TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataWithOmniboxExtension) {
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com"));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com"));
   std::string fake_id("blahblahblah");
@@ -433,6 +434,43 @@ TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataWithExtension) {
   }
 }
 
+TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataWithSearchOverrideExtension) {
+  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com"));
+  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com"));
+
+  // Change default search provider to a extension one.

[Peter Kasting, 2017/01/06 01:44:57]

Nit: a -> an

[Alexander Yashkin, 2017/01/07 12:55:59]

My mistake, done.

+  std::unique_ptr<TemplateURLData> extension =
+      GenerateDummyTemplateURLData("extension");
+  auto ext_dse = base::MakeUnique<TemplateURL>(
+      *extension, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto ext_info = base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("ext");
+  ext_info->wants_to_be_default_engine = true;
+  test_util_a_->AddExtensionControlledTURL(std::move(ext_dse),
+                                           std::move(ext_info));
+
+  const TemplateURL* ext_turl = model()->GetDefaultSearchProvider();
+  EXPECT_TRUE(model()->IsExtensionControlledDefaultSearch());
+
+  // Extension default search must not be synced across browsers.
+  syncer::SyncDataList all_sync_data =
+      model()->GetAllSyncData(syncer::SEARCH_ENGINES);
+

[Peter Kasting, 2017/01/06 01:44:57]

Nit: Blank line not necessary

[Alexander Yashkin, 2017/01/07 12:55:59]

Removed

+  EXPECT_EQ(2U, all_sync_data.size());
+
+  for (syncer::SyncDataList::const_iterator iter = all_sync_data.begin();
+       iter != all_sync_data.end(); ++iter) {

[Peter Kasting, 2017/01/06 01:44:57]

Nit: Range-based for?

[Alexander Yashkin, 2017/01/07 12:55:59]

Done

+    std::string guid = GetGUID(*iter);
+    const TemplateURL* service_turl = model()->GetTemplateURLForGUID(guid);
+    std::unique_ptr<TemplateURL> deserialized = Deserialize(*iter);
+    AssertEquals(*service_turl, *deserialized);
+    EXPECT_NE(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
+              deserialized->type());
+    EXPECT_NE(ext_turl->keyword(), deserialized->keyword());
+    EXPECT_NE(ext_turl->short_name(), deserialized->short_name());
+    EXPECT_NE(ext_turl->url(), deserialized->url());
+  }
+}
+
 TEST_F(TemplateURLServiceSyncTest, GetAllSyncDataNoManagedEngines) {
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com"));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com"));
@@ -1706,6 +1744,72 @@ TEST_F(TemplateURLServiceSyncTest, SyncWithManagedDefaultSearch) {
   EXPECT_EQ(expected_default, model()->GetDefaultSearchProvider());
 }
 
+TEST_F(TemplateURLServiceSyncTest, SyncWithExtensionDefaultSearch) {
+  // First start off with a few entries and make sure we can set an extension
+  // default search provider.
+  syncer::SyncDataList initial_data = CreateInitialSyncData();
+  model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
+                                    PassProcessor(),
+                                    CreateAndPassSyncErrorFactory());
+  model()->SetUserSelectedDefaultSearchProvider(
+      model()->GetTemplateURLForGUID("key2"));
+
+  // Expect one change because of user default engine change.
+  EXPECT_EQ(1U, processor()->change_list_size());

[Peter Kasting, 2017/01/06 01:44:57]

Nit: Consider doing:

  const size_t pending_changes = processor()->change_list_size();
  EXPECT_EQ(1U, pending_changes);

This way, down below, you can compare to |pending_changes| instead of 1U to make
it clearer the extension change should have left the existing number unaffected.
 The same comment applies to the "3U" for the number of affected engines.

[Alexander Yashkin, 2017/01/07 12:55:59]

Done.

+  ASSERT_TRUE(processor()->contains_guid("key2"));
+  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
+            processor()->change_for_guid("key2").change_type());
+
+  EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  ASSERT_TRUE(model()->GetDefaultSearchProvider());
+
+  // Change the default search provider to a extension one.

[Peter Kasting, 2017/01/06 01:44:57]

Nit: a -> an

[Alexander Yashkin, 2017/01/07 12:55:59]

Done.

+  std::unique_ptr<TemplateURLData> extension =
+      GenerateDummyTemplateURLData("extensiondefault");
+  auto ext_dse = base::MakeUnique<TemplateURL>(
+      *extension, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+  auto ext_info = base::MakeUnique<TemplateURL::AssociatedExtensionInfo>("ext");
+  ext_info->wants_to_be_default_engine = true;
+  test_util_a_->AddExtensionControlledTURL(std::move(ext_dse),
+                                           std::move(ext_info));
+
+  const TemplateURL* dsp_turl = model()->GetDefaultSearchProvider();
+  EXPECT_TRUE(model()->IsExtensionControlledDefaultSearch());
+
+  // Expect still one change because of user default engine change.

[Peter Kasting, 2017/01/06 01:44:57]

Nit: Maybe just "Extension-related changes to the DSE should not be synced as
search engine changes." plus the nit above about using temps instead of
hardcoded values here would be clearer + eliminate the need for the comment just
below.

[Alexander Yashkin, 2017/01/07 12:55:59]

Done.

+  EXPECT_EQ(1U, processor()->change_list_size());
+  // Check that added extension engine did not change number of engines
+  // in sync processor.
+  EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+
+  // Add a new entry from Sync. It should still sync in despite the default
+  // being extension controlled.
+  syncer::SyncChangeList changes;
+  changes.push_back(CreateTestSyncChange(
+      syncer::SyncChange::ACTION_ADD,
+      CreateTestTemplateURL(ASCIIToUTF16("newkeyword"),
+                            "http://new.com/{searchTerms}", "newdefault")));
+  model()->ProcessSyncChanges(FROM_HERE, changes);
+
+  EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+
+  // Change kSyncedDefaultSearchProviderGUID to point to the new entry and
+  // ensure that the DSP remains extension controlled.
+  profile_a()->GetTestingPrefService()->SetString(
+      prefs::kSyncedDefaultSearchProviderGUID, "newdefault");
+
+  EXPECT_EQ(dsp_turl, model()->GetDefaultSearchProvider());
+  EXPECT_TRUE(model()->IsExtensionControlledDefaultSearch());
+
+  // Remove extension DSE. Ensure that the DSP changes to the expected pending
+  // entry from Sync.
+  const TemplateURL* expected_default =
+      model()->GetTemplateURLForGUID("newdefault");
+  test_util_a_->RemoveExtensionControlledTURL("ext");
+
+  EXPECT_EQ(expected_default, model()->GetDefaultSearchProvider());
+}
+
 TEST_F(TemplateURLServiceSyncTest, SyncMergeDeletesDefault) {
   // If the value from Sync is a duplicate of the local default and is newer, it
   // should safely replace the local value and set as the new default.