[DRAFT] Allow a user to revert to their previous theme without closing infobar

chrome/browser/themes/theme_service.cc

Index: chrome/browser/themes/theme_service.cc
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
index d7fe33bd290520c6da71f222a288f2fe1bd3adbf..e6e3bbd26fb9ba79c589382c0347424d0dcdee52 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -55,6 +55,9 @@ namespace {
 // unpacked on the filesystem.)
 const char* kDefaultThemeGalleryID = "hkacjpbfdknhflllbcmjibkdeoafencn";
 
+// Wait this many seconds after startup to garbage collect unused themes.

[Yoyo Zhou, 2013/07/24 21:55:03]

Why wait?

+const int kRemoveUnusedThemesStartupDelay = 30;
+
 SkColor TintForUnderline(SkColor input) {
   return SkColorSetA(input, SkColorGetA(input) / 3);
 }
@@ -79,7 +82,9 @@ ThemeService::ThemeService()
     : rb_(ResourceBundle::GetSharedInstance()),
       profile_(NULL),
       ready_(false),
-      number_of_infobars_(0) {
+      number_of_infobars_(0),
+      installed_pending_load_id_(kDefaultThemeID),
+      weak_ptr_factory_(this) {
 }
 
 ThemeService::~ThemeService() {
@@ -99,6 +104,25 @@ void ThemeService::Init(Profile* profile) {
   }
 
   theme_syncable_service_.reset(new ThemeSyncableService(profile_, this));
+
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_INSTALLED,
+                 content::Source<Profile>(profile_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_LOADED,
+                 content::Source<Profile>(profile_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_ENABLED,
+                 content::Source<Profile>(profile_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                 content::Source<Profile>(profile_));
+
+  base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
+      base::Bind(&ThemeService::RemoveUnusedThemes,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 false),
+      base::TimeDelta::FromSeconds(kRemoveUnusedThemesStartupDelay));
 }
 
 gfx::Image ThemeService::GetImageNamed(int id) const {
@@ -229,39 +253,94 @@ base::RefCountedMemory* ThemeService::GetRawData(
 void ThemeService::Observe(int type,
                            const content::NotificationSource& source,
                            const content::NotificationDetails& details) {
-  DCHECK(type == chrome::NOTIFICATION_EXTENSIONS_READY);
-  registrar_.Remove(this, chrome::NOTIFICATION_EXTENSIONS_READY,
-      content::Source<Profile>(profile_));
+  using content::Details;
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSIONS_READY:
+      registrar_.Remove(this, chrome::NOTIFICATION_EXTENSIONS_READY,
+          content::Source<Profile>(profile_));
 
-  MigrateTheme();
-  set_ready();
+      MigrateTheme();
+      set_ready();
 
-  // Send notification in case anyone requested data and cached it when the
-  // theme service was not ready yet.
-  NotifyThemeChanged();
+      // Send notification in case anyone requested data and cached it when the
+      // theme service was not ready yet.
+      NotifyThemeChanged();
+      break;
+    case chrome::NOTIFICATION_EXTENSION_INSTALLED:
+    {
+      Details<const extensions::InstalledExtensionInfo> installed_details(
+          details);
+      // The theme may be initially disabled. Wait till it is loaded (if ever)

[Yoyo Zhou, 2013/07/24 21:55:03]

I don't see why we need to listen for extension installs or enables
specifically. See comment below.

[pkotwicz, 2013/07/25 01:25:29]

You are right. I should be able to move registering for the
NOTIFICATION_EXTENSION_LOADED notification and I can avoid listening to
NOTIFICATION_EXTENSION_INSTALLED and NOTIFICATION_EXTENSION_ENABLED as a result.

As background the ThemeService is created before the ExtensionService is inited
but after ExtensionSystem is created.
It can also be used before the ExtensionService has finished initing which makes
things ugly.

+      // to change the current theme.
+      if (installed_details->extension->is_theme())
+        installed_pending_load_id_ = installed_details->extension->id();
+      break;
+    }
+    case chrome::NOTIFICATION_EXTENSION_LOADED:
+    {
+      const Extension* extension = Details<const Extension>(details).ptr();
+      if (extension->is_theme() &&
+          installed_pending_load_id_ != kDefaultThemeID &&
+          installed_pending_load_id_ == extension->id()) {

[Yoyo Zhou, 2013/07/24 21:55:03]

What's the scenario where we load a theme and don't expect to set it? (When we
enable an extension, it also gets loaded.) Whenever this would happen, the theme
would be enabled and yet not active, which seems to be what you're trying to
fix.

+        SetTheme(extension);
+      }
+      installed_pending_load_id_ = kDefaultThemeID;
+    }
+    case chrome::NOTIFICATION_EXTENSION_ENABLED:
+    {
+      const Extension* extension = Details<const Extension>(details).ptr();
+      if (extension->is_theme())
+        SetTheme(extension);
+      break;
+    }
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED:
+    {
+      Details<const extensions::UnloadedExtensionInfo> unloaded_details(
+          details);
+      if (unloaded_details->reason != extension_misc::UNLOAD_REASON_UPDATE &&
+          unloaded_details->extension->is_theme() &&
+          unloaded_details->extension->id() == GetThemeID()) {
+        UseDefaultTheme();
+      }
+      break;
+    }
+  }
 }
 
 void ThemeService::SetTheme(const Extension* extension) {
-  // Clear our image cache.
-  FreePlatformCaches();
-
-  DCHECK(extension);
   DCHECK(extension->is_theme());
-  if (DCHECK_IS_ON()) {
-    ExtensionService* service =
-        extensions::ExtensionSystem::Get(profile_)->extension_service();
-    DCHECK(service);
-    DCHECK(service->GetExtensionById(extension->id(), false));
+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
+  if (!service->IsExtensionEnabled(extension->id())) {
+    // |extension| is disabled when reverting to the previous theme via an
+    // infobar.
+    service->EnableExtension(extension->id());
+    // Enabling the extension will call back to SetTheme().
+    return;
   }
 
+  std::string previous_theme_id = GetThemeID();
+
+  // Clear our image cache.
+  FreePlatformCaches();
+
   BuildFromExtension(extension);
   SaveThemeID(extension->id());
 
   NotifyThemeChanged();
   content::RecordAction(UserMetricsAction("Themes_Installed"));
+
+  if (previous_theme_id != kDefaultThemeID &&
+      previous_theme_id != extension->id()) {
+    // Disable the old theme.
+    service->DisableExtension(previous_theme_id,
+                              extensions::Extension::DISABLE_USER_ACTION);
+  }
 }
 
-void ThemeService::RemoveUnusedThemes() {
+void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
+  if (!ignore_infobars && number_of_infobars_ != 0)
+    return;
   if (!profile_)
     return;
   ExtensionService* service = profile_->GetExtensionService();
@@ -276,6 +355,13 @@ void ThemeService::RemoveUnusedThemes() {
       remove_list.push_back((*it)->id());
     }
   }
+  extensions = service->disabled_extensions();
+  for (ExtensionSet::const_iterator it = extensions->begin();
+       it != extensions->end(); ++it) {
+    if ((*it)->is_theme() && (*it)->id() != current_theme) {
+      remove_list.push_back((*it)->id());
+    }
+  }
   for (size_t i = 0; i < remove_list.size(); ++i)
     service->UninstallExtension(remove_list[i], false, NULL);
 }
@@ -322,7 +408,12 @@ void ThemeService::ClearAllThemeData() {
   profile_->GetPrefs()->ClearPref(prefs::kCurrentThemePackFilename);
   SaveThemeID(kDefaultThemeID);
 
-  RemoveUnusedThemes();
+  // There should be no more infobars. This may not be the case because of
+  // http://crbug.com/62154
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&ThemeService::RemoveUnusedThemes,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 true));
 }
 
 void ThemeService::LoadThemePrefs() {
@@ -448,7 +539,7 @@ void ThemeService::OnInfobarDestroyed() {
   number_of_infobars_--;
 
   if (number_of_infobars_ == 0)
-    RemoveUnusedThemes();
+    RemoveUnusedThemes(false);
 }
 
 ThemeSyncableService* ThemeService::GetThemeSyncableService() const {