Make extensions DSE persistent in browser prefs (Reland)

chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc

Index: chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
diff --git a/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc b/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
index d03d2e622d8487ac14348880368a5794d0d29849..a86f3ddfe4ff90613a028ff28f99f94d9f3515c5 100644
--- a/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
+++ b/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
@@ -9,135 +9,240 @@
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/search_engines/ui_thread_search_terms_data.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/test/base/search_test_utils.h"
 #include "components/prefs/pref_service.h"
+#include "components/search_engines/default_search_manager.h"
+#include "components/search_engines/search_engines_test_util.h"
 #include "components/search_engines/template_url.h"
+#include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/version_info/version_info.h"
 #include "extensions/common/features/feature_channel.h"
 
 namespace {
+#if defined(OS_WIN) || defined(OS_MACOSX)
+// Prepopulated id hardcoded in test_extension.
+const int kTestExtensionPrepopulatedId = 1;
+// TemplateURLData with search engines settings from test extension manifest.
+// chrome/test/data/extensions/settings_override/manifest.json
+std::unique_ptr<TemplateURLData> TestExtensionSearchEngine(PrefService* prefs) {
+  auto result = base::MakeUnique<TemplateURLData>();
+  result->SetShortName(base::ASCIIToUTF16("name.de"));
+  result->SetKeyword(base::ASCIIToUTF16("keyword.de"));
+  result->SetURL("http://www.foo.de/s?q={searchTerms}&id=10");
+  result->favicon_url = GURL("http://www.foo.de/favicon.ico?id=10");
+  result->suggestions_url = "http://www.foo.de/suggest?q={searchTerms}&id=10";
+  result->instant_url = "http://www.foo.de/instant?q={searchTerms}&id=10";
+  result->image_url = "http://www.foo.de/image?q={searchTerms}&id=10";
+  result->search_url_post_params = "search_lang=de";
+  result->suggestions_url_post_params = "suggest_lang=de";
+  result->instant_url_post_params = "instant_lang=de";
+  result->image_url_post_params = "image_lang=de";
+  result->alternate_urls.push_back("http://www.moo.de/s?q={searchTerms}&id=10");
+  result->alternate_urls.push_back("http://www.noo.de/s?q={searchTerms}&id=10");
+  result->input_encodings.push_back("UTF-8");
 
-class TemplateURLServiceObserver {
- public:
-  TemplateURLServiceObserver(TemplateURLService* service,
-                             base::RunLoop* loop)
-      : runner_(loop) {
-    DCHECK(loop);
-    template_url_sub_ = service->RegisterOnLoadedCallback(
-        base::Bind(&TemplateURLServiceObserver::StopLoop,
-                   base::Unretained(this)));
-    service->Load();
-  }
-  ~TemplateURLServiceObserver() {}
-
- private:
-  void StopLoop() {
-    runner_->Quit();
-  }
-  base::RunLoop* runner_;
-  std::unique_ptr<TemplateURLService::Subscription> template_url_sub_;
-
-  DISALLOW_COPY_AND_ASSIGN(TemplateURLServiceObserver);
-};
+  std::unique_ptr<TemplateURLData> prepopulated =
+      TemplateURLPrepopulateData::GetPrepopulatedEngine(
+          prefs, kTestExtensionPrepopulatedId);
+  // Values below do not exist in extension manifest and are taken from
+  // prepopulated engine with prepopulated_id set in extension manifest.
+  result->search_terms_replacement_key =
+      prepopulated->search_terms_replacement_key;
+  result->contextual_search_url = prepopulated->contextual_search_url;
+  result->new_tab_url = prepopulated->new_tab_url;
+  return result;
+}
 
 testing::AssertionResult VerifyTemplateURLServiceLoad(
     TemplateURLService* service) {
   if (service->loaded())
     return testing::AssertionSuccess();
-  base::RunLoop runner;
-  TemplateURLServiceObserver observer(service, &runner);
-  runner.Run();
+  search_test_utils::WaitForTemplateURLServiceToLoad(service);
   if (service->loaded())
     return testing::AssertionSuccess();
   return testing::AssertionFailure() << "TemplateURLService isn't loaded";
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideSettings) {
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) {
   PrefService* prefs = profile()->GetPrefs();
   ASSERT_TRUE(prefs);
   prefs->SetString(prefs::kHomePage, "http://google.com/");
   prefs->SetBoolean(prefs::kHomePageIsNewTabPage, true);
+
+  const extensions::Extension* extension = LoadExtensionWithInstallParam(
+      test_data_dir_.AppendASCII("settings_override"), kFlagEnableFileAccess,
+      "10");
+  ASSERT_TRUE(extension);
+  EXPECT_EQ("http://www.homepage.de/?param=10",
+            prefs->GetString(prefs::kHomePage));
+  EXPECT_FALSE(prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
+  UnloadExtension(extension->id());
+  EXPECT_EQ("http://google.com/", prefs->GetString(prefs::kHomePage));
+  EXPECT_TRUE(prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideStartupPagesSettings) {
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_TRUE(prefs);
   const GURL urls[] = {GURL("http://foo"), GURL("http://bar")};
   SessionStartupPref startup_pref(SessionStartupPref::LAST);
   startup_pref.urls.assign(urls, urls + arraysize(urls));
   SessionStartupPref::SetStartupPref(prefs, startup_pref);
-  TemplateURLService* url_service =
-      TemplateURLServiceFactory::GetForProfile(profile());
-  ASSERT_TRUE(url_service);
-  EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));
-  TemplateURL* default_provider = url_service->GetDefaultSearchProvider();
-  ASSERT_TRUE(default_provider);
-  EXPECT_EQ(TemplateURL::NORMAL, default_provider->type());
-
-#if defined(OS_MACOSX)
-  // On Mac, this API is limited to trunk.
-  extensions::ScopedCurrentChannel scoped_channel(
-      version_info::Channel::UNKNOWN);
-#endif  // OS_MACOSX
 
-#if defined(OS_WIN) || defined(OS_MACOSX)
   const extensions::Extension* extension = LoadExtensionWithInstallParam(
       test_data_dir_.AppendASCII("settings_override"),
       kFlagEnableFileAccess,
       "10");
   ASSERT_TRUE(extension);
-  EXPECT_EQ("http://www.homepage.de/?param=10",
-            prefs->GetString(prefs::kHomePage));
-  EXPECT_FALSE(prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
   startup_pref = SessionStartupPref::GetStartupPref(prefs);
   EXPECT_EQ(SessionStartupPref::URLS, startup_pref.type);
   EXPECT_EQ(std::vector<GURL>(1, GURL("http://www.startup.de/?param=10")),
             startup_pref.urls);
-  TemplateURL* extension_provider = url_service->GetDefaultSearchProvider();
-  EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
-            extension_provider->type());
-  EXPECT_EQ(base::ASCIIToUTF16("name.de"), extension_provider->short_name());
-  EXPECT_EQ(base::ASCIIToUTF16("keyword.de"), extension_provider->keyword());
-  EXPECT_EQ("http://www.foo.de/s?q={searchTerms}&id=10",
-            extension_provider->url());
-  EXPECT_EQ(GURL("http://www.foo.de/favicon.ico?id=10"),
-            extension_provider->favicon_url());
-  EXPECT_EQ("http://www.foo.de/suggest?q={searchTerms}&id=10",
-            extension_provider->suggestions_url());
-  EXPECT_EQ("http://www.foo.de/instant?q={searchTerms}&id=10",
-            extension_provider->instant_url());
-  EXPECT_EQ("http://www.foo.de/image?q={searchTerms}&id=10",
-            extension_provider->image_url());
-  EXPECT_EQ("search_lang=de", extension_provider->search_url_post_params());
-  EXPECT_EQ("suggest_lang=de",
-            extension_provider->suggestions_url_post_params());
-  EXPECT_EQ("instant_lang=de", extension_provider->instant_url_post_params());
-  EXPECT_EQ("image_lang=de", extension_provider->image_url_post_params());
-  const std::string alternate_urls[] = {
-      "http://www.moo.de/s?q={searchTerms}&id=10",
-      "http://www.noo.de/s?q={searchTerms}&id=10"};
-  EXPECT_EQ(std::vector<std::string>(
-                alternate_urls, alternate_urls + arraysize(alternate_urls)),
-            extension_provider->alternate_urls());
-  EXPECT_EQ(std::vector<std::string>(1, "UTF-8"),
-            extension_provider->input_encodings());
-  EXPECT_EQ(default_provider->new_tab_url(), extension_provider->new_tab_url());
-
   UnloadExtension(extension->id());
-  EXPECT_EQ("http://google.com/", prefs->GetString(prefs::kHomePage));
-  EXPECT_TRUE(prefs->GetBoolean(prefs::kHomePageIsNewTabPage));
   startup_pref = SessionStartupPref::GetStartupPref(prefs);
   EXPECT_EQ(SessionStartupPref::LAST, startup_pref.type);
   EXPECT_EQ(std::vector<GURL>(urls, urls + arraysize(urls)), startup_pref.urls);
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideDSE) {
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_TRUE(prefs);
+  TemplateURLService* url_service =
+      TemplateURLServiceFactory::GetForProfile(profile());
+  ASSERT_TRUE(url_service);
+  EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));
+  TemplateURL* default_provider = url_service->GetDefaultSearchProvider();
+  ASSERT_TRUE(default_provider);
+  EXPECT_EQ(TemplateURL::NORMAL, default_provider->type());
+
+  const extensions::Extension* extension = LoadExtensionWithInstallParam(
+      test_data_dir_.AppendASCII("settings_override"), kFlagEnableFileAccess,
+      "10");
+  ASSERT_TRUE(extension);
+  TemplateURL* current_dse = url_service->GetDefaultSearchProvider();
+  EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, current_dse->type());
+
+  std::unique_ptr<TemplateURLData> extension_dse =
+      TestExtensionSearchEngine(prefs);
+  ExpectSimilar(extension_dse.get(), &current_dse->data());
+
+  UnloadExtension(extension->id());
   EXPECT_EQ(default_provider, url_service->GetDefaultSearchProvider());
+}
+
+// Install and load extension into test profile.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PRE_OverridenDSEPersists) {
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_TRUE(prefs);
+  TemplateURLService* url_service =
+      TemplateURLServiceFactory::GetForProfile(profile());
+  ASSERT_TRUE(url_service);
+  EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));
+  TemplateURL* default_provider = url_service->GetDefaultSearchProvider();
+  ASSERT_TRUE(default_provider);
+  // Check that default provider is normal before extension is
+  // installed and loaded.
+  EXPECT_EQ(TemplateURL::NORMAL, default_provider->type());
+  EXPECT_NE(base::ASCIIToUTF16("name.de"), default_provider->short_name());
+  EXPECT_NE(base::ASCIIToUTF16("keyword.de"), default_provider->keyword());
+
+  // Install extension that overrides DSE.
+  const extensions::Extension* extension = LoadExtensionWithInstallParam(
+      test_data_dir_.AppendASCII("settings_override"), kFlagEnableFileAccess,
+      "10");
+  ASSERT_TRUE(extension);
+}
+
+// PRE_OverridenDSEPersists already installed extension with overriden DSE into
+// profile. Current test checks that after extension is installed,
+// default_search_manager correctly determines extension overriden DSE
+// from profile.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverridenDSEPersists) {
+  Profile* profile = browser()->profile();
+  DefaultSearchManager default_manager(
+      profile->GetPrefs(), DefaultSearchManager::ObserverCallback());
+
+  DefaultSearchManager::Source source;
+  const TemplateURLData* current_dse =
+      default_manager.GetDefaultSearchEngine(&source);
+
+  ASSERT_TRUE(current_dse);
+  std::unique_ptr<TemplateURLData> extension_dse =
+      TestExtensionSearchEngine(profile->GetPrefs());
+  ExpectSimilar(extension_dse.get(), current_dse);
+  EXPECT_EQ(DefaultSearchManager::FROM_EXTENSION, source);
+
+  // Check that new tab url is correctly overriden by extension.
+  TemplateURL ext_turl(*extension_dse,
+                       TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
+
+  std::string new_tab_url_ext = ext_turl.new_tab_url_ref().ReplaceSearchTerms(
+      TemplateURLRef::SearchTermsArgs(base::string16()),
+      UIThreadSearchTermsData(profile));
+
+  EXPECT_EQ(new_tab_url_ext, search::GetNewTabPageURL(profile).spec());
+
+  // Check that after template url service is loaded, extension dse persists.
+  TemplateURLService* url_service =
+      TemplateURLServiceFactory::GetForProfile(profile);
+  ASSERT_TRUE(url_service);
+  EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));
+  EXPECT_TRUE(url_service->IsExtensionControlledDefaultSearch());
+  TemplateURL* default_provider = url_service->GetDefaultSearchProvider();
+  ASSERT_TRUE(default_provider);
+  ExpectSimilar(extension_dse.get(), &default_provider->data());
+
+  UninstallExtension(
+      default_provider->GetExtensionInfoForTesting()->extension_id);
+}
+
+// This tests checks that extension overriding default search can be correctly
+// loaded before TemplateURLService is loaded.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BeforeTemplateUrlServiceLoad) {
+  PrefService* prefs = profile()->GetPrefs();
+  ASSERT_TRUE(prefs);
+  TemplateURLService* url_service =
+      TemplateURLServiceFactory::GetForProfile(profile());
+  ASSERT_TRUE(url_service);
+  EXPECT_TRUE(VerifyTemplateURLServiceLoad(url_service));

[vasilii, 2017/02/27 15:20:35]

What's going on here? Why you load the service in this test?

[Alexander Yashkin, 2017/02/27 19:36:03]

I decided to ensure that TemplateURLService is loaded first before setting it
explicitly to unloaded state.
This is browser test so TemplateURLService for profile could be loaded or not
loaded at the moment this test start to execute.
It depends on the other tasks performed before current test, if they call
TemplateURLServiceFactory::GetForProfile and DB task will execute 
before current task, TemplateURLService could be already loaded at the moment
BeforeTemplateUrlServiceLoad is executed.

I could be wrong, so please correct me if so.

[vasilii, 2017/02/28 14:59:41]

It's a browser test and what happens here should match the real behavior. There
is no way TemplateUrlService may handle the results from the database and switch
the flag back. 

[Alexander Yashkin, 2017/02/28 19:30:29]

Removed explicit TemplateURLService load and set_loaded(false).

+  // Set TemplateURLService unloaded for testing purpose.
+  url_service->set_loaded(false);

[vasilii, 2017/02/27 15:20:35]

I don't think it's applicable here in the browser test as all the keywords were
already loaded.

[Alexander Yashkin, 2017/02/27 19:36:03]

Why not applicable? I think set_loaded() function is very useful in this class,
no matter if keywords were loaded or not. 
Just nearly every function in TemplateURLService has check for loaded_ flag and
acts differently depending on its value.

This test could catch the source of flakiness that we spend a lot of time
tracing in current CL earlier.
It checks that if extension is loaded/unloaded while TemplateURLService is in
not-loaded state,
extension controlled default search engine is set and removed correctly.

[vasilii, 2017/02/28 14:59:42]

TemplateURLService::Load is called explicitly. If you say that it can be either
loaded or not here then which component is calling the method?
If it's critical for you that TemplateURLServiceLoad isn't loaded then it should
be done either by suppressing Load() or delaying the result delivery.
For me it's not that critical as the test should pass either way.

[Alexander Yashkin, 2017/02/28 19:30:29]

Removed TemplateURLService load.

+
+  EXPECT_FALSE(url_service->IsExtensionControlledDefaultSearch());
+  const extensions::Extension* extension = LoadExtensionWithInstallParam(
+      test_data_dir_.AppendASCII("settings_override"), kFlagEnableFileAccess,
+      "10");
+  ASSERT_TRUE(extension);
+  TemplateURL* current_dse = url_service->GetDefaultSearchProvider();
+  EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, current_dse->type());
+  EXPECT_TRUE(url_service->IsExtensionControlledDefaultSearch());
+
+  std::unique_ptr<TemplateURLData> extension_dse =
+      TestExtensionSearchEngine(prefs);
+  ExpectSimilar(extension_dse.get(), &current_dse->data());
+
+  UnloadExtension(extension->id());
+  EXPECT_NE(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
+            url_service->GetDefaultSearchProvider()->type());
+  EXPECT_FALSE(url_service->IsExtensionControlledDefaultSearch());
+}
+
 #else
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, SettingsOverridesDisallowed) {
   const extensions::Extension* extension =
       LoadExtensionWithFlags(test_data_dir_.AppendASCII("settings_override"),
                              kFlagIgnoreManifestWarnings);
   ASSERT_TRUE(extension);
   ASSERT_EQ(1u, extension->install_warnings().size());
-  EXPECT_EQ(std::string(
-                "'chrome_settings_overrides' "
-                "is not allowed for specified platform."),
+  EXPECT_EQ(std::string("'chrome_settings_overrides' "
+                        "is not allowed for specified platform."),
             extension->install_warnings().front().message);
-#endif
 }
+#endif
 
 }  // namespace