[Chrome colors] Move 3p NTP check to SearchTabHelper

In order to reuse ChromeColorsService in non-NTP contexts, we need to
move the 3p NTP check to the code that can be hit by 3p NTPs but not
by non-NTP clients. SearchTabHelper handles requests from the local
NTP renderer, so it's a natural fit.

The observer for NTP-provider changes was actually redundant, so this
CL removes it. Theme changes are reverted when the NTP navigates away
without calling ConfirmThemeChanges. When the user changes the
NTP-provider to third-party, all Google-NTP will be either:
- closed which will trigger the theme revert (old NTP)
- keep the same state so the user can continue modifying the theme
(webUI NTP)

Bug: 1128451
Change-Id: I3a72ce9ffbaccca580b52e1b43f5b348e9bb9feb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411944
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807954}

chrome/browser/search/chrome_colors/chrome_colors_service.cc

@@ -5,7 +5,6 @@
 #include "chrome/browser/search/chrome_colors/chrome_colors_service.h"
 
 #include "base/metrics/histogram_macros.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/common/search/generated_colors_info.h"
 #include "chrome/common/search/selected_colors_info.h"
@@ -13,18 +12,7 @@
 namespace chrome_colors {
 
 ChromeColorsService::ChromeColorsService(Profile* profile)
-    : theme_service_(ThemeServiceFactory::GetForProfile(profile)) {
-  // Determine if we are using a third-party NTP. When user switches to
-  // third-party NTP we should revert all the changes.
-  TemplateURLService* template_url_service =
-      TemplateURLServiceFactory::GetForProfile(profile);
-  if (template_url_service) {
-    search_provider_observer_ = std::make_unique<SearchProviderObserver>(
-        template_url_service,
-        base::BindRepeating(&ChromeColorsService::OnSearchProviderChanged,
-                            weak_ptr_factory_.GetWeakPtr()));
-  }
-}
+    : theme_service_(ThemeServiceFactory::GetForProfile(profile)) {}
 
 ChromeColorsService::~ChromeColorsService() = default;
 
@@ -46,37 +34,27 @@ void ChromeColorsService::RecordColorOnLoadHistogram(SkColor color) {
 }
 
 void ChromeColorsService::ApplyDefaultTheme(content::WebContents* tab) {
-  if (!search_provider_observer_ || !search_provider_observer_->is_google())
-    return;
   SaveThemeRevertState(tab);
   theme_service_->UseDefaultTheme();
 }
 
 void ChromeColorsService::ApplyAutogeneratedTheme(SkColor color,
                                                   content::WebContents* tab) {
-  if (!search_provider_observer_ || !search_provider_observer_->is_google())
-    return;
   SaveThemeRevertState(tab);
   theme_service_->BuildAutogeneratedThemeFromColor(color);
 }
 
 void ChromeColorsService::RevertThemeChangesForTab(content::WebContents* tab) {
-  if (!search_provider_observer_ || !search_provider_observer_->is_google() ||
-      dialog_tab_ != tab) {
+  if (dialog_tab_ != tab)
     return;
-  }
   RevertThemeChangesInternal();
 }
 
 void ChromeColorsService::RevertThemeChanges() {
-  if (!search_provider_observer_ || !search_provider_observer_->is_google())
-    return;
   RevertThemeChangesInternal();
 }
 
 void ChromeColorsService::ConfirmThemeChanges() {
-  if (!search_provider_observer_ || !search_provider_observer_->is_google())
-    return;
   prev_theme_reinstaller_ = nullptr;
   dialog_tab_ = nullptr;
 }
@@ -89,11 +67,6 @@ void ChromeColorsService::RevertThemeChangesInternal() {
   }
 }
 
-void ChromeColorsService::OnSearchProviderChanged() {
-  if (search_provider_observer_ && !search_provider_observer_->is_google())
-    RevertThemeChangesInternal();
-}
-
 void ChromeColorsService::SaveThemeRevertState(content::WebContents* tab) {
   // TODO(crbug.com/980745): Support theme reverting for multiple tabs.
   if (!prev_theme_reinstaller_) {

chrome/browser/search/chrome_colors/chrome_colors_service.h

@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_SEARCH_CHROME_COLORS_CHROME_COLORS_SERVICE_H_
 
 #include "base/callback.h"
-#include "chrome/browser/search/search_provider_observer.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/web_contents.h"
@@ -57,9 +56,6 @@ class ChromeColorsService : public KeyedService {
   // Reverts to the previous theme state unconditionally.
   void RevertThemeChangesInternal();
 
-  // Callback for search provider change.
-  void OnSearchProviderChanged();
-
   // Saves the necessary state(revert callback and the current tab) for
   // performing theme change revert. Saves the state only if it is not set.
   void SaveThemeRevertState(content::WebContents* tab);
@@ -75,9 +71,6 @@ class ChromeColorsService : public KeyedService {
   // Used for reverting back to the previously installed theme.
   std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller_;
 
-  // Keeps track of any changes in search engine provider. May be null.
-  std::unique_ptr<SearchProviderObserver> search_provider_observer_;
-
   base::WeakPtrFactory<ChromeColorsService> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ChromeColorsService);

chrome/browser/search/chrome_colors/chrome_colors_service_unittest.cc

@@ -5,15 +5,12 @@
 #include "chrome/browser/search/chrome_colors/chrome_colors_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/search/chrome_colors/chrome_colors_factory.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/search/local_ntp_test_utils.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/search_test_utils.h"
-#include "components/search_engines/template_url.h"
-#include "components/search_engines/template_url_data.h"
-#include "components/search_engines/template_url_service.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -24,9 +21,6 @@ class TestChromeColorsService : public BrowserWithTestWindowTest {
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
 
-    template_url_service_ = TemplateURLServiceFactory::GetForProfile(profile());
-    search_test_utils::WaitForTemplateURLServiceToLoad(template_url_service_);
-
     chrome_colors_service_ =
         chrome_colors::ChromeColorsFactory::GetForProfile(profile());
 
@@ -38,33 +32,8 @@ class TestChromeColorsService : public BrowserWithTestWindowTest {
     return !!chrome_colors_service_->prev_theme_reinstaller_;
   }
 
-  void SetUserSelectedDefaultSearchProvider(const std::string& base_url) {
-    TemplateURLData data;
-    data.SetShortName(base::UTF8ToUTF16(base_url));
-    data.SetKeyword(base::UTF8ToUTF16(base_url));
-    data.SetURL(base_url + "url?bar={searchTerms}");
-    data.new_tab_url = base_url + "newtab";
-    data.alternate_urls.push_back(base_url + "alt#quux={searchTerms}");
-
-    TemplateURL* template_url =
-        template_url_service_->Add(std::make_unique<TemplateURL>(data));
-    template_url_service_->SetUserSelectedDefaultSearchProvider(template_url);
-  }
-
   chrome_colors::ChromeColorsService* chrome_colors_service_;
   content::WebContents* tab_;
-
- private:
-  // BrowserWithTestWindowTest override:
-  TestingProfile* CreateProfile() override {
-    TestingProfile* profile = BrowserWithTestWindowTest::CreateProfile();
-    TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-        profile,
-        base::BindRepeating(&TemplateURLServiceFactory::BuildInstanceFor));
-    return profile;
-  }
-
-  TemplateURLService* template_url_service_;
 };
 
 TEST_F(TestChromeColorsService, ApplyAndConfirmAutogeneratedTheme) {
@@ -203,23 +172,3 @@ TEST_F(TestChromeColorsService, RevertThemeChangesForTab) {
   EXPECT_FALSE(theme_service->UsingAutogeneratedTheme());
   EXPECT_FALSE(HasThemeReinstaller());
 }
-
-TEST_F(TestChromeColorsService, RevertThemeChangesWhenSwitchToThirdPartyNTP) {
-  ThemeService* theme_service = ThemeServiceFactory::GetForProfile(profile());
-  ASSERT_TRUE(theme_service->UsingDefaultTheme());
-
-  SkColor theme_color = SkColorSetRGB(100, 0, 200);
-  chrome_colors_service_->ApplyAutogeneratedTheme(theme_color, tab_);
-  EXPECT_TRUE(theme_service->UsingAutogeneratedTheme());
-  EXPECT_TRUE(HasThemeReinstaller());
-
-  // Switching to third-party NTP should revert current changes.
-  SetUserSelectedDefaultSearchProvider("www.third-party-ntp.com");
-  EXPECT_FALSE(theme_service->UsingAutogeneratedTheme());
-  EXPECT_FALSE(HasThemeReinstaller());
-
-  // When third-party NTP is present autogenerated theme shouldn't apply.
-  chrome_colors_service_->ApplyAutogeneratedTheme(theme_color, tab_);
-  EXPECT_FALSE(theme_service->UsingAutogeneratedTheme());
-  EXPECT_FALSE(HasThemeReinstaller());
-}

chrome/browser/ui/search/search_tab_helper.cc

@@ -572,23 +572,31 @@ void SearchTabHelper::OnOptOutOfSearchSuggestions() {
 }
 
 void SearchTabHelper::OnApplyDefaultTheme() {
-  if (chrome_colors_service_)
+  if (chrome_colors_service_ &&
+      search::DefaultSearchProviderIsGoogle(profile())) {
     chrome_colors_service_->ApplyDefaultTheme(web_contents_);
+  }
 }
 
 void SearchTabHelper::OnApplyAutogeneratedTheme(SkColor color) {
-  if (chrome_colors_service_)
+  if (chrome_colors_service_ &&
+      search::DefaultSearchProviderIsGoogle(profile())) {
     chrome_colors_service_->ApplyAutogeneratedTheme(color, web_contents_);
+  }
 }
 
 void SearchTabHelper::OnRevertThemeChanges() {
-  if (chrome_colors_service_)
+  if (chrome_colors_service_ &&
+      search::DefaultSearchProviderIsGoogle(profile())) {
     chrome_colors_service_->RevertThemeChanges();
+  }
 }
 
 void SearchTabHelper::OnConfirmThemeChanges() {
-  if (chrome_colors_service_)
+  if (chrome_colors_service_ &&
+      search::DefaultSearchProviderIsGoogle(profile())) {
     chrome_colors_service_->ConfirmThemeChanges();
+  }
 }
 
 void SearchTabHelper::QueryAutocomplete(const base::string16& input,

chrome/browser/ui/webui/customize_themes/chrome_customize_themes_handler_unittest.cc

@@ -16,7 +16,6 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/test_extension_environment.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/common/chrome_paths.h"
@@ -129,18 +128,13 @@ MATCHER(MatchesColorInfo, "") {
 class ChromeCustomizeThemesHandlerTest : public testing::Test {
  public:
   ChromeCustomizeThemesHandlerTest()
-      : web_contents_(factory_.CreateWebContents(profile())) {}
-
-  void SetUp() override {
-    TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
-        profile(),
-        base::BindRepeating(&TemplateURLServiceFactory::BuildInstanceFor));
-    handler_ = std::make_unique<ChromeCustomizeThemesHandler>(
-        mock_client_.BindAndGetRemote(),
-        mojo::PendingReceiver<
-            customize_themes::mojom::CustomizeThemesHandler>(),
-        web_contents_, profile());
-  }
+      : web_contents_(factory_.CreateWebContents(profile())),
+        handler_(std::make_unique<ChromeCustomizeThemesHandler>(
+            mock_client_.BindAndGetRemote(),
+            mojo::PendingReceiver<
+                customize_themes::mojom::CustomizeThemesHandler>(),
+            web_contents_,
+            profile())) {}
 
   void TearDown() override {
     if (handler_) {