Uninstall themes when all theme infobars are gone; not on each infobar's destruction.

There are multiple problems with the current implementation:
- It handles multiple tabs with theme install infobars badly.
- It has a subtle race condition where installing a second theme, and then
  installing the original theme (note: not clicking undo) uninstalled both
  themes.

Instead, only uninstall unused themes when all infobars are closed down.

BUG=none
TEST=Install theme A. Install theme B. Install theme A. Restart chrome. Theme A should show.
TEST=Install theme A in window 1. Install theme B in window 2. Install theme C in window 3. Clicking undo in any of those windows in any order should work.

Review URL: http://codereview.chromium.org/504052

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34890 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 37 insertions, 29 deletions

diff --git a/chrome/browser/browser_theme_provider.cc b/chrome/browser/browser_theme_provider.cc
index 338e42e..ae08fa8 100644
--- a/chrome/browser/browser_theme_provider.cc
+++ b/chrome/browser/browser_theme_provider.cc
@@ -179,7 +179,8 @@ bool BrowserThemeProvider::IsThemeableImage(int resource_id) {
 
 BrowserThemeProvider::BrowserThemeProvider()
     : rb_(ResourceBundle::GetSharedInstance()),
-      profile_(NULL) {
+      profile_(NULL),
+      number_of_infobars_(0) {
   // Initialize the themeable image map so we can use it on other threads.
   HasThemeableImage(0);
 }
@@ -572,3 +573,14 @@ void BrowserThemeProvider::BuildFromExtension(Extension* extension) {
   SavePackName(pack_path);
   theme_pack_ = pack;
 }
+
+void BrowserThemeProvider::OnInfobarDisplayed() {
+  number_of_infobars_++;
+}
+
+void BrowserThemeProvider::OnInfobarDestroyed() {
+  number_of_infobars_--;
+
+  if (number_of_infobars_ == 0)
+    RemoveUnusedThemes();
+}
diff --git a/chrome/browser/browser_theme_provider.h b/chrome/browser/browser_theme_provider.h
index e6275d6..756e2bc 100644
--- a/chrome/browser/browser_theme_provider.h
+++ b/chrome/browser/browser_theme_provider.h
@@ -129,6 +129,14 @@ class BrowserThemeProvider : public NonThreadSafe,
   // locally customized.)
   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();
+
   // Convert a bitfield alignment into a string like "top left". Public so that
   // it can be used to generate CSS values. Takes a bitfield of AlignmentMasks.
   static std::string AlignmentToString(int alignment);
@@ -215,6 +223,9 @@ class BrowserThemeProvider : public NonThreadSafe,
 
   scoped_refptr<BrowserThemePack> theme_pack_;
 
+  // The number of infobars currently displayed.
+  int number_of_infobars_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowserThemeProvider);
 };
 
diff --git a/chrome/browser/dom_ui/dom_ui.cc b/chrome/browser/dom_ui/dom_ui.cc
index 34c5956..5017a9f 100644
--- a/chrome/browser/dom_ui/dom_ui.cc
+++ b/chrome/browser/dom_ui/dom_ui.cc
@@ -9,6 +9,7 @@
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/values.h"
+#include "chrome/browser/browser_theme_provider.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/tab_contents/tab_contents_view.h"
diff --git a/chrome/browser/extensions/gtk_theme_installed_infobar_delegate.cc b/chrome/browser/extensions/gtk_theme_installed_infobar_delegate.cc
index efca805..d3fc1c0 100644
--- a/chrome/browser/extensions/gtk_theme_installed_infobar_delegate.cc
+++ b/chrome/browser/extensions/gtk_theme_installed_infobar_delegate.cc
@@ -16,8 +16,6 @@ GtkThemeInstalledInfoBarDelegate::GtkThemeInstalledInfoBarDelegate(
 }
 
 bool GtkThemeInstalledInfoBarDelegate::Cancel() {
-  was_canceled_ = true;
-
   if (previous_use_gtk_theme_) {
     profile()->SetNativeTheme();
     return true;
diff --git a/chrome/browser/extensions/theme_installed_infobar_delegate.cc b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
index caea07d..07072c0 100644
--- a/chrome/browser/extensions/theme_installed_infobar_delegate.cc
+++ b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
@@ -7,6 +7,7 @@
 #include "app/l10n_util.h"
 #include "app/resource_bundle.h"
 #include "base/string_util.h"
+#include "chrome/browser/browser_theme_provider.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
@@ -18,28 +19,17 @@ ThemeInstalledInfoBarDelegate::ThemeInstalledInfoBarDelegate(
     TabContents* tab_contents, const Extension* new_theme,
     const std::string& previous_theme_id)
          : ConfirmInfoBarDelegate(tab_contents),
-           was_canceled_(false),
            profile_(tab_contents->profile()),
            name_(new_theme->name()),
-           new_theme_id_(new_theme->id()),
            previous_theme_id_(previous_theme_id) {
+  profile_->GetThemeProvider()->OnInfobarDisplayed();
+}
+
+ThemeInstalledInfoBarDelegate::~ThemeInstalledInfoBarDelegate() {
+  profile_->GetThemeProvider()->OnInfobarDestroyed();
 }
 
 void ThemeInstalledInfoBarDelegate::InfoBarClosed() {
-  ExtensionsService* service = profile_->GetExtensionsService();
-  // Only delete the theme if we've installed a new theme and not the same
-  // theme on top of the current one.
-  if (service && previous_theme_id_ != new_theme_id_) {
-    std::string uninstall_id;
-    if (was_canceled_)
-      uninstall_id = new_theme_id_;
-    else if (!previous_theme_id_.empty())
-      uninstall_id = previous_theme_id_;
-    // It's possible that the theme was already uninstalled by someone so make
-    // sure it exists.
-    if (!uninstall_id.empty() && service->GetExtensionById(uninstall_id, true))
-      service->UninstallExtension(uninstall_id, false);
-  }
   delete this;
 }
 
@@ -78,7 +68,6 @@ std::wstring ThemeInstalledInfoBarDelegate::GetButtonLabel(
 }
 
 bool ThemeInstalledInfoBarDelegate::Cancel() {
-  was_canceled_ = true;
   if (!previous_theme_id_.empty()) {
     ExtensionsService* service = profile_->GetExtensionsService();
     if (service) {
diff --git a/chrome/browser/extensions/theme_installed_infobar_delegate.h b/chrome/browser/extensions/theme_installed_infobar_delegate.h
index daa11e6..b8baf41 100644
--- a/chrome/browser/extensions/theme_installed_infobar_delegate.h
+++ b/chrome/browser/extensions/theme_installed_infobar_delegate.h
@@ -18,6 +18,7 @@ class ThemeInstalledInfoBarDelegate : public ConfirmInfoBarDelegate {
   ThemeInstalledInfoBarDelegate(TabContents* tab_contents,
                                 const Extension* new_theme,
                                 const std::string& previous_theme_id);
+  virtual ~ThemeInstalledInfoBarDelegate();
   virtual void InfoBarClosed();
   virtual std::wstring GetMessageText() const;
   virtual SkBitmap* GetIcon() const;
@@ -30,15 +31,10 @@ class ThemeInstalledInfoBarDelegate : public ConfirmInfoBarDelegate {
  protected:
   Profile* profile() { return profile_; }
 
-  // Keeps track of whether we canceled the install or not.
-  bool was_canceled_;
-
  private:
   Profile* profile_;
   // Name of theme that's just been installed.
   std::string name_;
-  // Id of theme that's just been installed.
-  std::string new_theme_id_;
   // Used to undo theme install.
   std::string previous_theme_id_;
 };
diff --git a/chrome/browser/gtk/tabs/dragged_tab_gtk.cc b/chrome/browser/gtk/tabs/dragged_tab_gtk.cc
index 40127ca..12e9639 100644
--- a/chrome/browser/gtk/tabs/dragged_tab_gtk.cc
+++ b/chrome/browser/gtk/tabs/dragged_tab_gtk.cc
@@ -11,6 +11,7 @@
 #include "app/gfx/canvas_paint.h"
 #include "app/gfx/gtk_util.h"
 #include "app/l10n_util.h"
+#include "chrome/browser/browser_theme_provider.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index e61ae5a..7b1b80e 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -464,7 +464,7 @@ class OffTheRecordProfileImpl : public Profile,
     return GetOriginalProfile()->GetTheme();
   }
 
-  virtual ThemeProvider* GetThemeProvider() {
+  virtual BrowserThemeProvider* GetThemeProvider() {
     return GetOriginalProfile()->GetThemeProvider();
   }
 
@@ -1228,7 +1228,7 @@ Extension* ProfileImpl::GetTheme() {
   return extensions_service_->GetExtensionById(id, false);
 }
 
-ThemeProvider* ProfileImpl::GetThemeProvider() {
+BrowserThemeProvider* ProfileImpl::GetThemeProvider() {
   InitThemes();
   return theme_provider_.get();
 }
diff --git a/chrome/browser/profile.h b/chrome/browser/profile.h
index e72dbd7..95f76b0 100644
--- a/chrome/browser/profile.h
+++ b/chrome/browser/profile.h
@@ -272,7 +272,7 @@ class Profile {
   virtual Extension* GetTheme() = 0;
 
   // Returns or creates the ThemeProvider associated with this profile
-  virtual ThemeProvider* GetThemeProvider() = 0;
+  virtual BrowserThemeProvider* GetThemeProvider() = 0;
 
   virtual ThumbnailStore* GetThumbnailStore() = 0;
 
@@ -435,7 +435,7 @@ class ProfileImpl : public Profile,
   virtual void SetNativeTheme();
   virtual void ClearTheme();
   virtual Extension* GetTheme();
-  virtual ThemeProvider* GetThemeProvider();
+  virtual BrowserThemeProvider* GetThemeProvider();
   virtual ThumbnailStore* GetThumbnailStore();
   virtual bool HasCreatedDownloadManager() const;
   virtual URLRequestContextGetter* GetRequestContext();