[Themes] Uninstall themes only if there are no reinstallers.

Previously unused themes were uninstalled when no infobars were present.
But now when themes can be reverted ChromeColorsService, counting
infobars is not sufficient for preventing uninstall of necessary themes.

This CL adds ThemeReinstaller class that will update the of number of
existing reinstallers and will call RemoveUnusedThemes when there are
no reinstallers left.

This CL also removes ignore_infobars param from RemoveUnusedThemes as it
is not relevant anymore.


Bug: 985001, 991715
Change-Id: Ia3905ee2d8c5fb238c8d6a380d17ed9d3018e2a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1757067Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Commit-Queue: Gayane Petrosyan <gayane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688367}

chrome/browser/extensions/theme_installed_infobar_delegate.cc

@@ -29,14 +29,14 @@ void ThemeInstalledInfoBarDelegate::Create(
     ThemeService* theme_service,
     const std::string& theme_name,
     const std::string& theme_id,
-    base::OnceClosure revert_theme_callback) {
+    std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller) {
   // Create the new infobar.
   std::unique_ptr<infobars::InfoBar> new_infobar(
       infobar_service->CreateConfirmInfoBar(
           std::unique_ptr<ConfirmInfoBarDelegate>(
               new ThemeInstalledInfoBarDelegate(
                   theme_service, theme_name, theme_id,
-                  std::move(revert_theme_callback)))));
+                  std::move(prev_theme_reinstaller)))));
 
   // If there's a previous theme infobar, just replace that instead of adding a
   // new one.
@@ -52,27 +52,24 @@ void ThemeInstalledInfoBarDelegate::Create(
       // don't show an infobar, it's valid in this case.
       if (theme_infobar->theme_id_ != theme_id) {
         infobar_service->ReplaceInfoBar(old_infobar, std::move(new_infobar));
-        theme_service->OnInfobarDisplayed();
       }
       return;
     }
   }
-
   // No previous theme infobar, so add this.
   infobar_service->AddInfoBar(std::move(new_infobar));
-  theme_service->OnInfobarDisplayed();
 }
 
 ThemeInstalledInfoBarDelegate::ThemeInstalledInfoBarDelegate(
     ThemeService* theme_service,
     const std::string& theme_name,
     const std::string& theme_id,
-    base::OnceClosure revert_theme_callback)
+    std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller)
     : ConfirmInfoBarDelegate(),
       theme_service_(theme_service),
       theme_name_(theme_name),
       theme_id_(theme_id),
-      revert_theme_callback_(std::move(revert_theme_callback)) {
+      prev_theme_reinstaller_(std::move(prev_theme_reinstaller)) {
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
                  content::Source<ThemeService>(theme_service_));
 }
@@ -80,8 +77,6 @@ ThemeInstalledInfoBarDelegate::ThemeInstalledInfoBarDelegate(
 ThemeInstalledInfoBarDelegate::~ThemeInstalledInfoBarDelegate() {
   // We don't want any notifications while we're running our destructor.
   registrar_.RemoveAll();
-
-  theme_service_->OnInfobarDestroyed();
 }
 
 infobars::InfoBarDelegate::InfoBarIdentifier
@@ -114,8 +109,8 @@ base::string16 ThemeInstalledInfoBarDelegate::GetButtonLabel(
 }
 
 bool ThemeInstalledInfoBarDelegate::Cancel() {
-  if (!revert_theme_callback_.is_null())
-    std::move(revert_theme_callback_).Run();
+  if (prev_theme_reinstaller_)
+    prev_theme_reinstaller_->Reinstall();
   return false;  // The theme change will close us.
 }
 

chrome/browser/extensions/theme_installed_infobar_delegate.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "chrome/browser/themes/theme_service.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -16,7 +17,6 @@
 #include "third_party/skia/include/core/SkColor.h"
 
 class InfoBarService;
-class ThemeService;
 
 // When a user installs a theme, we display it immediately, but provide an
 // infobar allowing them to cancel.
@@ -25,17 +25,19 @@ class ThemeInstalledInfoBarDelegate : public ConfirmInfoBarDelegate,
  public:
   // Creates a theme installed infobar and delegate and adds the infobar to
   // |infobar_service|, replacing any previous theme infobar.
-  static void Create(InfoBarService* infobar_service,
-                     ThemeService* theme_service,
-                     const std::string& theme_name,
-                     const std::string& theme_id,
-                     base::OnceClosure revert_theme_callback);
+  static void Create(
+      InfoBarService* infobar_service,
+      ThemeService* theme_service,
+      const std::string& theme_name,
+      const std::string& theme_id,
+      std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller);
 
  private:
-  ThemeInstalledInfoBarDelegate(ThemeService* theme_service,
-                                const std::string& theme_name,
-                                const std::string& theme_id,
-                                base::OnceClosure revert_theme_callback);
+  ThemeInstalledInfoBarDelegate(
+      ThemeService* theme_service,
+      const std::string& theme_name,
+      const std::string& theme_id,
+      std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller);
   ~ThemeInstalledInfoBarDelegate() override;
 
   // ConfirmInfoBarDelegate:
@@ -61,7 +63,7 @@ class ThemeInstalledInfoBarDelegate : public ConfirmInfoBarDelegate,
   std::string theme_id_;
 
   // Used to undo theme install.
-  base::OnceClosure revert_theme_callback_;
+  std::unique_ptr<ThemeService::ThemeReinstaller> prev_theme_reinstaller_;
 
   // Registers and unregisters us for notifications.
   content::NotificationRegistrar registrar_;

chrome/browser/infobars/infobars_browsertest.cc

@@ -269,7 +269,9 @@ void InfoBarUiTest::ShowUi(const std::string& name) {
       ThemeInstalledInfoBarDelegate::Create(
           GetInfoBarService(),
           ThemeServiceFactory::GetForProfile(browser()->profile()), "New Theme",
-          "id", base::OnceClosure());
+          "id",
+          std::make_unique<ThemeService::ThemeReinstaller>(
+              browser()->profile(), base::OnceClosure()));
       break;
 
     case IBD::NACL_INFOBAR_DELEGATE:

chrome/browser/search/chrome_colors/chrome_colors_service.cc

@@ -85,15 +85,15 @@ void ChromeColorsService::RevertThemeChanges() {
 void ChromeColorsService::ConfirmThemeChanges() {
   if (!search_provider_observer_ || !search_provider_observer_->is_google())
     return;
-  revert_theme_changes_.Reset();
+  prev_theme_reinstaller_ = nullptr;
   dialog_tab_ = nullptr;
   RecordChangesConfirmedHistogram(true);
 }
 
 void ChromeColorsService::RevertThemeChangesWithReason(RevertReason reason) {
-  if (!revert_theme_changes_.is_null()) {
-    std::move(revert_theme_changes_).Run();
-    revert_theme_changes_.Reset();
+  if (prev_theme_reinstaller_) {
+    prev_theme_reinstaller_->Reinstall();
+    prev_theme_reinstaller_ = nullptr;
     dialog_tab_ = nullptr;
     UMA_HISTOGRAM_ENUMERATION("ChromeColors.RevertReason", reason);
     RecordChangesConfirmedHistogram(false);
@@ -107,8 +107,8 @@ void ChromeColorsService::OnSearchProviderChanged() {
 
 void ChromeColorsService::SaveThemeRevertState(content::WebContents* tab) {
   // TODO(crbug.com/980745): Support theme reverting for multiple tabs.
-  if (revert_theme_changes_.is_null()) {
-    revert_theme_changes_ = theme_service_->GetRevertThemeCallback();
+  if (!prev_theme_reinstaller_) {
+    prev_theme_reinstaller_ = theme_service_->BuildReinstallerForCurrentTheme();
     dialog_tab_ = tab;
   }
 }

chrome/browser/search/chrome_colors/chrome_colors_service.h

@@ -83,9 +83,8 @@ class ChromeColorsService : public KeyedService {
   // The first tab that used Apply* and hasn't Confirm/Revert the changes.
   content::WebContents* dialog_tab_ = nullptr;
 
-  // Callback that will revert the theme to the state it was at the time of this
-  // callback's creation.
-  base::OnceClosure revert_theme_changes_;
+  // 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_;

chrome/browser/search/chrome_colors/chrome_colors_service_unittest.cc

@@ -34,8 +34,8 @@ class TestChromeColorsService : public BrowserWithTestWindowTest {
     tab_ = browser()->tab_strip_model()->GetActiveWebContents();
   }
 
-  bool HasThemeRevertCallback() {
-    return !chrome_colors_service_->revert_theme_changes_.is_null();
+  bool HasThemeReinstaller() {
+    return !!chrome_colors_service_->prev_theme_reinstaller_;
   }
 
   void SetUserSelectedDefaultSearchProvider(const std::string& base_url) {
@@ -74,18 +74,18 @@ TEST_F(TestChromeColorsService, ApplyAndConfirmAutogeneratedTheme) {
   SkColor theme_color1 = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color1, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   SkColor theme_color2 = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color2, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   // Last color is saved.
   chrome_colors_service_->ConfirmThemeChanges();
   EXPECT_TRUE(theme_service->UsingAutogenerated());
   EXPECT_EQ(theme_color2, theme_service->GetThemeColor());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService, ApplyAndRevertAutogeneratedTheme) {
@@ -95,17 +95,17 @@ TEST_F(TestChromeColorsService, ApplyAndRevertAutogeneratedTheme) {
   SkColor theme_color1 = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color1, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   SkColor theme_color2 = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color2, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   // State before first apply is restored.
   chrome_colors_service_->RevertThemeChanges();
   EXPECT_FALSE(theme_service->UsingAutogenerated());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService,
@@ -118,12 +118,12 @@ TEST_F(TestChromeColorsService,
   SkColor new_theme_color = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(new_theme_color, tab_);
   EXPECT_EQ(new_theme_color, theme_service->GetThemeColor());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->ConfirmThemeChanges();
   EXPECT_TRUE(theme_service->UsingAutogenerated());
   EXPECT_EQ(new_theme_color, theme_service->GetThemeColor());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService,
@@ -136,12 +136,12 @@ TEST_F(TestChromeColorsService,
   SkColor new_theme_color = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(new_theme_color, tab_);
   EXPECT_EQ(new_theme_color, theme_service->GetThemeColor());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->RevertThemeChanges();
   EXPECT_TRUE(theme_service->UsingAutogenerated());
   EXPECT_EQ(prev_theme_color, theme_service->GetThemeColor());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService, ApplyAndConfirmDefaultTheme_withPreviousTheme) {
@@ -153,12 +153,12 @@ TEST_F(TestChromeColorsService, ApplyAndConfirmDefaultTheme_withPreviousTheme) {
 
   chrome_colors_service_->ApplyDefaultTheme(tab_);
   EXPECT_TRUE(theme_service->UsingDefaultTheme());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->ConfirmThemeChanges();
   EXPECT_TRUE(theme_service->UsingDefaultTheme());
   EXPECT_NE(prev_theme_color, theme_service->GetThemeColor());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService, ApplyAndRevertDefaultTheme_withPreviousTheme) {
@@ -170,12 +170,12 @@ TEST_F(TestChromeColorsService, ApplyAndRevertDefaultTheme_withPreviousTheme) {
 
   chrome_colors_service_->ApplyDefaultTheme(tab_);
   EXPECT_TRUE(theme_service->UsingDefaultTheme());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->RevertThemeChanges();
   EXPECT_FALSE(theme_service->UsingDefaultTheme());
   EXPECT_EQ(prev_theme_color, theme_service->GetThemeColor());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService, RevertThemeChangesForTab) {
@@ -185,11 +185,11 @@ TEST_F(TestChromeColorsService, RevertThemeChangesForTab) {
   SkColor theme_color = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->RevertThemeChangesForTab(nullptr);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   AddTab(browser(), GURL("chrome://newtab"));
   content::WebContents* second_tab =
@@ -197,11 +197,11 @@ TEST_F(TestChromeColorsService, RevertThemeChangesForTab) {
   ASSERT_NE(tab_, second_tab);
   chrome_colors_service_->RevertThemeChangesForTab(second_tab);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   chrome_colors_service_->RevertThemeChangesForTab(tab_);
   EXPECT_FALSE(theme_service->UsingAutogenerated());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }
 
 TEST_F(TestChromeColorsService, RevertThemeChangesWhenSwitchToThirdPartyNTP) {
@@ -211,15 +211,15 @@ TEST_F(TestChromeColorsService, RevertThemeChangesWhenSwitchToThirdPartyNTP) {
   SkColor theme_color = SkColorSetRGB(100, 0, 200);
   chrome_colors_service_->ApplyAutogeneratedTheme(theme_color, tab_);
   EXPECT_TRUE(theme_service->UsingAutogenerated());
-  EXPECT_TRUE(HasThemeRevertCallback());
+  EXPECT_TRUE(HasThemeReinstaller());
 
   // Switching to third-party NTP should revert current changes.
   SetUserSelectedDefaultSearchProvider("www.third-party-ntp.com");
   EXPECT_FALSE(theme_service->UsingAutogenerated());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  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->UsingAutogenerated());
-  EXPECT_FALSE(HasThemeRevertCallback());
+  EXPECT_FALSE(HasThemeReinstaller());
 }

chrome/browser/themes/theme_service.cc

@@ -307,6 +307,25 @@ class ThemeService::ThemeObserver
 };
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
+// ThemeService::ThemeReinstaller -----------------------------------------
+
+ThemeService::ThemeReinstaller::ThemeReinstaller(Profile* profile,
+                                                 base::OnceClosure installer)
+    : theme_service_(ThemeServiceFactory::GetForProfile(profile)) {
+  theme_service_->number_of_reinstallers_++;
+  installer_ = std::move(installer);
+}
+
+ThemeService::ThemeReinstaller::~ThemeReinstaller() {
+  theme_service_->number_of_reinstallers_--;
+  theme_service_->RemoveUnusedThemes();
+}
+
+void ThemeService::ThemeReinstaller::Reinstall() {
+  if (!installer_.is_null()) {
+    std::move(installer_).Run();
+  }
+}
 
 // ThemeService ---------------------------------------------------------------
 
@@ -321,7 +340,7 @@ ThemeService::ThemeService()
       rb_(ui::ResourceBundle::GetSharedInstance()),
       profile_(nullptr),
       installed_pending_load_id_(kDefaultThemeID),
-      number_of_infobars_(0),
+      number_of_reinstallers_(0),
       original_theme_provider_(*this, false, false),
       incognito_theme_provider_(*this, true, false),
       default_theme_provider_(*this, false, true) {}
@@ -467,26 +486,14 @@ std::string ThemeService::GetThemeID() const {
   return profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);
 }
 
-void ThemeService::OnInfobarDisplayed() {
-  number_of_infobars_++;
-}
-
-void ThemeService::OnInfobarDestroyed() {
-  number_of_infobars_--;
-
-  if (number_of_infobars_ == 0 &&
-      !build_extension_task_tracker_.HasTrackedTasks()) {
-    RemoveUnusedThemes(false);
-  }
-}
-
-void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
+void ThemeService::RemoveUnusedThemes() {
   // We do not want to garbage collect themes on startup (|ready_| is false).
   // Themes will get garbage collected after |kRemoveUnusedThemesStartupDelay|.
   if (!profile_ || !ready_)
     return;
-  if (!ignore_infobars && number_of_infobars_ != 0)
+  if (number_of_reinstallers_ != 0 || !building_extension_id_.empty()) {
     return;
+  }
 
   extensions::ExtensionService* service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
@@ -502,8 +509,7 @@ void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
   for (extensions::ExtensionSet::const_iterator it = extensions->begin();
        it != extensions->end(); ++it) {
     const extensions::Extension* extension = it->get();
-    if (extension->is_theme() && extension->id() != current_theme &&
-        extension->id() != building_extension_id_) {
+    if (extension->is_theme() && extension->id() != current_theme) {
       // Only uninstall themes which are not disabled or are disabled with
       // reason DISABLE_USER_ACTION. We cannot blanket uninstall all disabled
       // themes because externally installed themes are initially disabled.
@@ -574,22 +580,31 @@ SkColor ThemeService::GetThemeColor() const {
   return profile_->GetPrefs()->GetInteger(prefs::kAutogeneratedThemeColor);
 }
 
-base::OnceCallback<void()> ThemeService::GetRevertThemeCallback() {
+std::unique_ptr<ThemeService::ThemeReinstaller>
+ThemeService::BuildReinstallerForCurrentTheme() {
+  base::OnceClosure reinstall_callback;
   const CustomThemeSupplier* theme_supplier = get_theme_supplier();
-  if (theme_supplier) {
-    const CustomThemeSupplier::ThemeType theme_type =
-        theme_supplier->get_theme_type();
-    if (theme_type == CustomThemeSupplier::ThemeType::EXTENSION) {
-      return base::BindOnce(&ThemeService::RevertToExtensionTheme,
-                            weak_ptr_factory_.GetWeakPtr(), GetThemeID());
-    } else if (theme_type == CustomThemeSupplier::ThemeType::AUTOGENERATED) {
-      return base::BindOnce(&ThemeService::BuildFromColor,
-                            weak_ptr_factory_.GetWeakPtr(), GetThemeColor());
-    }
+  if (theme_supplier && theme_supplier->get_theme_type() ==
+                            CustomThemeSupplier::ThemeType::EXTENSION) {
+    reinstall_callback =
+        base::BindOnce(&ThemeService::RevertToExtensionTheme,
+                       weak_ptr_factory_.GetWeakPtr(), GetThemeID());
+  } else if (theme_supplier &&
+             theme_supplier->get_theme_type() ==
+                 CustomThemeSupplier::ThemeType::AUTOGENERATED) {
+    reinstall_callback =
+        base::BindOnce(&ThemeService::BuildFromColor,
+                       weak_ptr_factory_.GetWeakPtr(), GetThemeColor());
+  } else if (UsingSystemTheme()) {
+    reinstall_callback = base::BindOnce(&ThemeService::UseSystemTheme,
+                                        weak_ptr_factory_.GetWeakPtr());
+  } else {
+    reinstall_callback = base::BindOnce(&ThemeService::UseDefaultTheme,
+                                        weak_ptr_factory_.GetWeakPtr());
   }
-  return base::BindOnce(UsingSystemTheme() ? &ThemeService::UseSystemTheme
-                                           : &ThemeService::UseDefaultTheme,
-                        weak_ptr_factory_.GetWeakPtr());
+
+  return std::make_unique<ThemeReinstaller>(profile_,
+                                            std::move(reinstall_callback));
 }
 
 void ThemeService::SetCustomDefaultTheme(
@@ -927,7 +942,7 @@ void ThemeService::OnExtensionServiceReady() {
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(&ThemeService::RemoveUnusedThemes,
-                     weak_ptr_factory_.GetWeakPtr(), false),
+                     weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(kRemoveUnusedThemesStartupDelay));
 }
 
@@ -1007,7 +1022,8 @@ void ThemeService::OnThemeBuiltFromExtension(
   extensions::GetExtensionFileTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&WritePackToDiskCallback,
                                 base::RetainedRef(pack), extension->path()));
-  base::OnceClosure callback = ThemeService::GetRevertThemeCallback();
+  std::unique_ptr<ThemeService::ThemeReinstaller> reinstaller =
+      BuildReinstallerForCurrentTheme();
   base::Optional<std::string> previous_theme_id;
   if (UsingExtensionTheme())
     previous_theme_id = GetThemeID();
@@ -1015,11 +1031,13 @@ void ThemeService::OnThemeBuiltFromExtension(
   SwapThemeSupplier(std::move(pack));
   SetThemePrefsForExtension(extension);
   NotifyThemeChanged();
+  building_extension_id_.clear();
 
   // Same old theme, but the theme has changed (migrated) or auto-updated.
   if (previous_theme_id.has_value() &&
-      previous_theme_id.value() == extension->id())
+      previous_theme_id.value() == extension->id()) {
     return;
+  }
 
   base::RecordAction(UserMetricsAction("Themes_Installed"));
 
@@ -1040,11 +1058,10 @@ void ThemeService::OnThemeBuiltFromExtension(
         ThemeInstalledInfoBarDelegate::Create(
             InfoBarService::FromWebContents(web_contents),
             ThemeServiceFactory::GetForProfile(profile_), extension->name(),
-            extension->id(), std::move(callback));
+            extension->id(), std::move(reinstaller));
       }
     }
   }
-  building_extension_id_.clear();
 }
 
 void ThemeService::ClearThemePrefs() {

chrome/browser/themes/theme_service.h

@@ -55,6 +55,22 @@ class ThemeService : public content::NotificationObserver,
                      public KeyedService,
                      public ui::NativeThemeObserver {
  public:
+  // This class keeps track of the number of existing |ThemeReinstaller|
+  // objects. When that number reaches 0 then unused themes will be deleted.
+  class ThemeReinstaller {
+   public:
+    ThemeReinstaller(Profile* profile, base::OnceClosure installer);
+    ~ThemeReinstaller();
+
+    void Reinstall();
+
+   private:
+    base::OnceClosure installer_;
+    ThemeService* const theme_service_;
+
+    DISALLOW_COPY_AND_ASSIGN(ThemeReinstaller);
+  };
+
   // Public constants used in ThemeService and its subclasses:
   static const char kDefaultThemeID[];
 
@@ -114,18 +130,8 @@ class ThemeService : public content::NotificationObserver,
   // locally customized.)
   virtual std::string GetThemeID() const;
 
-  // This class needs to keep track of the number of theme infobars so that we
-  // clean up unused themes.
-  void OnInfobarDisplayed();
-
-  // Decrements the number of theme infobars. If the last infobar has been
-  // destroyed, uninstalls all themes that aren't the currently selected.
-  void OnInfobarDestroyed();
-
-  // Uninstall theme extensions which are no longer in use. |ignore_infobars| is
-  // whether unused themes should be removed despite a theme infobar being
-  // visible.
-  void RemoveUnusedThemes(bool ignore_infobars);
+  // Uninstall theme extensions which are no longer in use.
+  void RemoveUnusedThemes();
 
   // Returns the syncable service for syncing theme. The returned service is
   // owned by |this| object.
@@ -149,8 +155,9 @@ class ThemeService : public content::NotificationObserver,
   virtual bool UsingAutogenerated() const;
   virtual SkColor GetThemeColor() const;
 
-  // Returns callback for reverting to previous theme.
-  base::OnceCallback<void()> GetRevertThemeCallback();
+  // Returns |ThemeService::ThemeReinstaller| for the current theme.
+  std::unique_ptr<ThemeService::ThemeReinstaller>
+  BuildReinstallerForCurrentTheme();
 
  protected:
   // Set a custom default theme instead of the normal default theme.
@@ -317,7 +324,7 @@ class ThemeService : public content::NotificationObserver,
   std::string installed_pending_load_id_;
 
   // The number of infobars currently displayed.
-  int number_of_infobars_;
+  int number_of_reinstallers_;
 
   // A cache of already-computed values for COLOR_TOOLBAR_TOP_SEPARATOR, which
   // can be expensive to compute.

chrome/browser/themes/theme_service_unittest.cc

@@ -177,8 +177,10 @@ TEST_F(ThemeServiceTest, DisableUnusedTheme) {
   EXPECT_EQ(extension1_id, theme_service->GetThemeID());
   EXPECT_TRUE(service_->IsExtensionEnabled(extension1_id));
 
-  // Show an infobar to prevent the current theme from being uninstalled.
-  theme_service->OnInfobarDisplayed();
+  // Create theme reinstaller to prevent the current theme from being
+  // uninstalled.
+  std::unique_ptr<ThemeService::ThemeReinstaller> reinstaller =
+      theme_service->BuildReinstallerForCurrentTheme();
 
   const std::string& extension2_id =
       LoadUnpackedMinimalThemeAt(temp_dir2.GetPath());
@@ -225,7 +227,8 @@ TEST_F(ThemeServiceTest, ThemeUpgrade) {
   // Let the ThemeService uninstall unused themes.
   base::RunLoop().RunUntilIdle();
 
-  theme_service->OnInfobarDisplayed();
+  std::unique_ptr<ThemeService::ThemeReinstaller> reinstaller =
+      theme_service->BuildReinstallerForCurrentTheme();
 
   base::ScopedTempDir temp_dir1;
   ASSERT_TRUE(temp_dir1.CreateUniqueTempDir());
@@ -407,39 +410,8 @@ TEST_F(ThemeServiceTest, NTPLogoAlternate) {
   }
 }
 
-namespace {
-
-// NotificationObserver which emulates an infobar getting destroyed when the
-// theme changes.
-class InfobarDestroyerOnThemeChange : public content::NotificationObserver {
- public:
-  explicit InfobarDestroyerOnThemeChange(Profile* profile)
-      : theme_service_(ThemeServiceFactory::GetForProfile(profile)) {
-    registrar_.Add(this, chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
-        content::Source<ThemeService>(theme_service_));
-  }
-
-  ~InfobarDestroyerOnThemeChange() override {}
-
- private:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override {
-    theme_service_->OnInfobarDestroyed();
-  }
-
-  // Not owned.
-  ThemeService* theme_service_;
-
-  content::NotificationRegistrar registrar_;
-
-  DISALLOW_COPY_AND_ASSIGN(InfobarDestroyerOnThemeChange);
-};
-
-}  // namespace
-
 // crbug.com/468280
-TEST_F(ThemeServiceTest, UninstallThemeOnThemeChangeNotification) {
+TEST_F(ThemeServiceTest, UninstallThemeWhenNoReinstallers) {
   // Setup.
   ThemeService* theme_service =
       ThemeServiceFactory::GetForProfile(profile_.get());
@@ -456,24 +428,31 @@ TEST_F(ThemeServiceTest, UninstallThemeOnThemeChangeNotification) {
       LoadUnpackedMinimalThemeAt(temp_dir1.GetPath());
   ASSERT_EQ(extension1_id, theme_service->GetThemeID());
 
-  // Show an infobar.
-  theme_service->OnInfobarDisplayed();
-
-  // Install another theme. The first extension shouldn't be uninstalled yet as
-  // it should be possible to revert to it. Emulate the infobar destroying
-  // itself as a result of the NOTIFICATION_BROWSER_THEME_CHANGED notification.
+  std::string extension2_id = "";
   {
-    InfobarDestroyerOnThemeChange destroyer(profile_.get());
-    const std::string& extension2_id =
-        LoadUnpackedMinimalThemeAt(temp_dir2.GetPath());
+    // Show an infobar.
+    std::unique_ptr<ThemeService::ThemeReinstaller> reinstaller =
+        theme_service->BuildReinstallerForCurrentTheme();
+
+    // Install another theme. The first extension shouldn't be uninstalled yet
+    // as it should be possible to revert to it.
+    extension2_id = LoadUnpackedMinimalThemeAt(temp_dir2.GetPath());
+    EXPECT_TRUE(registry_->GetExtensionById(extension1_id,
+                                            ExtensionRegistry::DISABLED));
     EXPECT_EQ(extension2_id, theme_service->GetThemeID());
+
+    reinstaller->Reinstall();
+    WaitForThemeInstall();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(registry_->GetExtensionById(extension2_id,
+                                            ExtensionRegistry::DISABLED));
+    EXPECT_EQ(extension1_id, theme_service->GetThemeID());
   }
 
-  // Check that it is possible to reinstall extension1.
-  ThemeServiceFactory::GetForProfile(profile_.get())
-      ->RevertToExtensionTheme(extension1_id);
-  WaitForThemeInstall();
-  EXPECT_EQ(extension1_id, theme_service->GetThemeID());
+  // extension 2 should get uninstalled as no reinstallers are in scope.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(registry_->GetExtensionById(extension2_id,
+                                           ExtensionRegistry::EVERYTHING));
 }
 
 TEST_F(ThemeServiceTest, BuildFromColorTest) {