Make extensions DSE persistent in browser prefs

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..34bcc7693a588a141d9c5ef76f1c9f98bdf2d2ce 100644
--- a/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
+++ b/chrome/browser/extensions/api/settings_overrides/settings_overrides_browsertest.cc
@@ -11,50 +11,58 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.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 {
+// TemplateURLData with search engines settings from test extension manifest.
+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();
+  std::unique_ptr<TemplateURLData> google_engine(
+      TemplateURLPrepopulateData::GetPrepopulatedEngine(prefs, 1));

[vasilii, 2016/11/30 14:00:02]

prepopulated_id should come from the manifest instead of hardcoding '1'.

[Alexander Yashkin, 2016/12/05 18:16:48]

Added extraction of prepopulated_id from manifest.

+  // Values below are taken for Google engine settings, because extension
+  // manifest has prepopulate_id 1, which is Google.
+  result->search_terms_replacement_key =
+      google_engine->search_terms_replacement_key;
+  result->contextual_search_url = google_engine->contextual_search_url;

[vasilii, 2016/11/30 14:00:02]

contextual_search_url and search_terms_replacement_key should be rather
overwritten in the manifest itself.

[Alexander Yashkin, 2016/12/05 18:16:48]

I don't think that someone would want to override contextual_search_url and
search_terms_replacement_key.
They are rather exotic, IMHO, and are used only by Google engine AFAIK.

I can add new_tab_url extraction to manifest parsing. 
new_tab_url is currently used by Google, Yandex and Bing search engines.
Do you think it should be done in this CL?

[vasilii, 2016/12/06 19:16:42]

No, that's fine.

+  result->new_tab_url = google_engine->new_tab_url;
+  return result;
   }
-  ~TemplateURLServiceObserver() {}
-
- private:
-  void StopLoop() {
-    runner_->Quit();
-  }
-  base::RunLoop* runner_;
-  std::unique_ptr<TemplateURLService::Subscription> template_url_sub_;
-
-  DISALLOW_COPY_AND_ASSIGN(TemplateURLServiceObserver);
-};
 
 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";
 }
+}  // namespace

[vasilii, 2016/11/30 14:00:02]

Why do you move the namespace?

[Alexander Yashkin, 2016/12/05 18:16:48]

Reverted, did not even noticed in hacking rage :)

 
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideSettings) {
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideHomePageSettings) {

[vasilii, 2016/11/30 14:00:02]

If you decided to split the tests then one should overwrite only home page and
the other one only DSE. Presumably, testing only the start-up pages is useful
too.

[Alexander Yashkin, 2016/12/05 18:16:48]

Ok, splitted home page and startup pages tests.

   PrefService* prefs = profile()->GetPrefs();
   ASSERT_TRUE(prefs);
   prefs->SetString(prefs::kHomePage, "http://google.com/");
@@ -63,13 +71,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideSettings) {
   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.

[vasilii, 2016/11/30 14:00:02]

I think it's not true anymore.

[Alexander Yashkin, 2016/12/05 18:16:48]

Deleted.

@@ -90,42 +91,54 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideSettings) {
   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);
+#else
+  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."),
+            extension->install_warnings().front().message);
+#endif
+}
+
+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());
+
+#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);
+  TemplateURL* extension_provider = url_service->GetDefaultSearchProvider();
+  EXPECT_EQ(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
+            extension_provider->type());
+
+  auto extension_dse = TestExtensionSearchEngine(prefs);
+  ExpectSimilar(&extension_provider->data(), extension_dse.get());
+
+  UnloadExtension(extension->id());
   EXPECT_EQ(default_provider, url_service->GetDefaultSearchProvider());
 #else
   const extensions::Extension* extension =
@@ -140,4 +153,50 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, OverrideSettings) {
 #endif
 }
 
-}  // namespace
+#if defined(OS_WIN) || defined(OS_MACOSX)
+// Install and load extension into test profile.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PRE_OverridenDSEPersists) {

[vasilii, 2016/11/30 14:00:02]

The test should load an extension that overwrites only the DSE. Then you have to
check the new tab page. That's what the bug is about.

[Alexander Yashkin, 2016/12/05 18:16:48]

new_tab page from extension overriden engine not applying to first tab is only
one of the consequences of bigger problem, IMHO.
The bigger problem is that extension overriden default search engine is not
applied at all, until extension subsystem is loaded.

Added new test extension that overrides DSE only and changed test to use it.
Not sure that new extension is needed. Whats wrong with one extension used by
both tests?

[vasilii, 2016/12/06 19:16:42]

I think you can reuse the extension.
You can make the test stronger by using NewTabURLDetails and verifying that it
reads the same new tab page that was set by the extension.

+  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());
+
+#if defined(OS_MACOSX)
+  // On Mac, this API is limited to trunk.
+  extensions::ScopedCurrentChannel scoped_channel(
+      version_info::Channel::UNKNOWN);
+#endif  // OS_MACOSX
+  // 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;
+  TemplateURLData* dse = default_manager.GetDefaultSearchEngine(&source);
+
+  ASSERT_TRUE(dse);
+  auto extension_dse = TestExtensionSearchEngine(profile->GetPrefs());
+  ExpectSimilar(dse, extension_dse.get());
+  EXPECT_EQ(DefaultSearchManager::FROM_EXTENSION, source);
+}
+#endif