[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 22e9e281713b5cdb01f4d4f8e3e4aab339b3db56..6867e3a475b07d14cb113ade825455f8e2ceaec1 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -57,6 +57,9 @@ namespace {
 // unpacked on the filesystem.)
 const char* kDefaultThemeGalleryID = "hkacjpbfdknhflllbcmjibkdeoafencn";
 
+// Wait this many seconds after startup to garbage collect unused themes.
+const int kRemoveUnusedThemesStartupDelay = 30;

[pkotwicz, 2013/07/29 06:12:35]

I wait here because we garbage collected extensions on a timeout.
ExtensionService::GarbageCollectExtensions.

[Yoyo Zhou, 2013/07/30 00:29:18]

Ok, please add a comment even if it's just "see ExtensionService::...".

[pkotwicz, 2013/07/31 21:42:52]

Done.

+
 SkColor TintForUnderline(SkColor input) {
   return SkColorSetA(input, SkColorGetA(input) / 3);
 }
@@ -81,7 +84,9 @@ ThemeService::ThemeService()
     : rb_(ResourceBundle::GetSharedInstance()),
       profile_(NULL),
       ready_(false),
-      number_of_infobars_(0) {
+      installed_pending_load_id_(kDefaultThemeID),
+      number_of_infobars_(0),
+      weak_ptr_factory_(this) {
 }
 
 ThemeService::~ThemeService() {
@@ -94,11 +99,9 @@ void ThemeService::Init(Profile* profile) {
 
   LoadThemePrefs();
 
-  if (!ready_) {
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_EXTENSIONS_READY,
-                   content::Source<Profile>(profile_));
-  }
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSIONS_READY,
+                 content::Source<Profile>(profile_));
 
   theme_syncable_service_.reset(new ThemeSyncableService(profile_, this));
 }
@@ -204,36 +207,83 @@ 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_));
-
-  MigrateTheme();
-  set_ready();
-
-  // Send notification in case anyone requested data and cached it when the
-  // theme service was not ready yet.
-  NotifyThemeChanged();
+  using content::Details;
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSIONS_READY:
+      registrar_.Remove(this, chrome::NOTIFICATION_EXTENSIONS_READY,
+          content::Source<Profile>(profile_));
+      OnExtensionServiceReady();
+      break;
+    case chrome::NOTIFICATION_EXTENSION_INSTALLED:
+    {
+      // The theme may be initially disabled. Wait till it is loaded (if ever).
+      Details<const extensions::InstalledExtensionInfo> installed_details(
+          details);
+      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() &&

[Yoyo Zhou, 2013/07/30 00:29:18]

I don't see which case we need to worry about extensions that are loaded but yet
were neither installed nor enabled.

I'm afraid there are going to be subtle bugs here. What if we have an external
theme, we install a theme from the webstore, and then try to accept the external
theme?

[pkotwicz, 2013/07/31 21:42:52]

- On ToT/stable it is not possible to enable an externally loaded theme.
- With this CL, the more complex scenario you described works.

The fact that it was never possible to install an externally loaded theme may
indicate that we should just disable externally loaded themes. (That would
probably be a different CL), but it would slightly simplify
ThemeService::RemoveUnusedThemes() in this CL.

The scenario I am concerned about: (Scenario B)
- A user installs a theme and exits chrome.
- The policy is changed to disable the theme the user has active. (Via setting
ExtensionInstallBlacklist)

User starts chrome, the now policy blacklisted theme is not loaded.
(ManagementPolicy::UserMayLoad returns false). The theme now sticks around for
ever. My concern is that if the theme is ever unblacklisted that it will be set.

Ideally, ThemeService::RemoveUnusedThemes() would clean those themes up but
making that change requires a lot more in depth knowledge of extensions than I
have.
Note that Scenario B is broken because despite the extension not being loaded,
the ThemeService still uses the management policy blacklisted theme. (This is a
bug with and without this CL)

[Yoyo Zhou, 2013/07/31 22:13:00]

Well, if we restart Chrome, we don't seem to call SetTheme on the existing
theme. (Besides, we don't listen to EXTENSION_LOADED here until after
EXTENSIONS_READY, which is after the initial load of extensions.) So it will
only be set if it goes through EnableExtension (I'm not sure if that happens -
kalman?). There may be other subtle bugs, though.

[not at google - send to devlin, 2013/07/31 22:19:13]

No idea off the top of my head, jyasskin is probably the most familiar with
extension loading stuff.

[pkotwicz, 2013/07/31 22:37:22]

- I have an idea as to how to fix the bug wrt to the theme being loaded if the
managemenet policy blacklisted the theme. (I will do that in a separate CL
though)
- yoz@, I am not too sure if I understand your most recent comment. I have not
found a way that a theme which is unblacklisted by the management policy can get
loaded after the EXTENSIONS_READY notification is sent. (I am concerned that a
future CL may change this)

+          installed_pending_load_id_ != kDefaultThemeID &&
+          installed_pending_load_id_ == extension->id()) {
+        SetTheme(extension);
+      }
+      installed_pending_load_id_ = kDefaultThemeID;
+      break;
+    }
+    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::SetCustomDefaultTheme(
@@ -247,12 +297,13 @@ bool ThemeService::ShouldInitWithNativeTheme() const {
   return false;
 }
 
-void ThemeService::RemoveUnusedThemes() {
+void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
   // We do not want to garbage collect themes on startup (|ready_| is false).
-  // Themes will get garbage collected once
-  // ExtensionService::GarbageCollectExtensions() runs.
+  // Themes will get garbage collected after |kRemoveUnusedThemesStartupDelay|.
   if (!profile_ || !ready_)
     return;
+  if (!ignore_infobars && number_of_infobars_ != 0)
+    return;
 
   ExtensionService* service = profile_->GetExtensionService();
   if (!service)
@@ -266,6 +317,25 @@ void ThemeService::RemoveUnusedThemes() {
       remove_list.push_back((*it)->id());
     }
   }
+  // Uninstall themes with disable reason DISABLE_USER_ACTION. We cannot blanket

[Yoyo Zhou, 2013/07/30 00:29:18]

Suppose you open Chrome and install a new theme, and you have an infobar for the
old one. Then 30s after startup this runs. What happens?

Ths might be an existing bug - it appears the old theme is gone, and it reverts
back to the default theme instead.

[pkotwicz, 2013/07/31 21:42:52]

This CL fixes that issue too. RemoveUnusedThemes() checks |number_of_infobars_|.

+  // uninstall all disabled themes because externally installed extensions are
+  // initially disabled.
+  extensions = service->disabled_extensions();
+  extensions::ExtensionPrefs* prefs = service->extension_prefs();
+  for (ExtensionSet::const_iterator it = extensions->begin();
+       it != extensions->end(); ++it) {
+    const extensions::Extension* extension = *it;
+    if (extension->is_theme() &&
+        extension->id() != current_theme &&
+        prefs->GetDisableReasons(extension->id()) &
+            Extension::DISABLE_USER_ACTION) {
+      remove_list.push_back((*it)->id());
+    }
+  }
+  // TODO: Garbage collects all unused themes. This method misses blacklisted
+  // themes and themes which are installed but not loaded because they are
+  // blacklisted by a management policy provider.

[Yoyo Zhou, 2013/07/30 00:29:18]

Seems like we can go over service->blacklisted_extensions() too. We shouldn't
end up with blacklisted themes that aren't being used.

[not at google - send to devlin, 2013/07/30 01:57:12]

there are two types of blacklists, management policy blacklist and the more
common google blacklist.

service->blacklist() is the latter.

I don't understand any of this stuff but it seems very complicated. Inspecting
the state of extensions like this is bound to lead to subtle bugs, can you look
at the installed extension set instead or something?

[pkotwicz, 2013/07/31 21:42:52]

Ideally, we would clean up themes in both blacklists (but that's outside of the
scope of this CL)

I now use ExtensionService::GenerateInstalledExtensionSet() which cuts down on
the number of loops.
kalman@, thanks for the suggestion!

+
   for (size_t i = 0; i < remove_list.size(); ++i)
     service->UninstallExtension(remove_list[i], false, NULL);
 }
@@ -320,7 +390,14 @@ 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
+  // RemoveUnusedThemes is called on a task because ClearAllThemeData() may
+  // be called as a result of NOTIFICATION_EXTENSION_UNLOADED.
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&ThemeService::RemoveUnusedThemes,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 true));
 }
 
 void ThemeService::LoadThemePrefs() {
@@ -351,16 +428,9 @@ void ThemeService::LoadThemePrefs() {
   if (loaded_pack) {
     content::RecordAction(UserMetricsAction("Themes.Loaded"));
     set_ready();
-  } else {
-    // TODO(erg): We need to pop up a dialog informing the user that their
-    // theme is being migrated.
-    ExtensionService* service =
-        extensions::ExtensionSystem::Get(profile_)->extension_service();
-    if (service && service->is_ready()) {
-      MigrateTheme();
-      set_ready();
-    }
   }
+  // Else: wait for the extension service to be ready so that the theme pack
+  // can be recreated from the extension.
 }
 
 void ThemeService::NotifyThemeChanged() {
@@ -390,16 +460,41 @@ void ThemeService::FreePlatformCaches() {
 }
 #endif
 
-void ThemeService::SwapThemeSupplier(
-    scoped_refptr<CustomThemeSupplier> theme_supplier) {
-  if (theme_supplier_.get())
-    theme_supplier_->StopUsingTheme();
-  theme_supplier_ = theme_supplier;
-  if (theme_supplier_.get())
-    theme_supplier_->StartUsingTheme();
+void ThemeService::OnExtensionServiceReady() {
+  if (!ready_) {
+    // If the ThemeService is not ready yet, the custom theme data pack needs to
+    // be recreated from the extension.
+    MigrateTheme();
+    set_ready();
+
+    // Send notification in case anyone requested data and cached it when the
+    // theme service was not ready yet.
+    NotifyThemeChanged();
+  }
+
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_INSTALLED,
+                 content::Source<Profile>(profile_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_ENABLED,
+                 content::Source<Profile>(profile_));
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_LOADED,
+                 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));
 }
 
 void ThemeService::MigrateTheme() {
+  // TODO(erg): We need to pop up a dialog informing the user that their
+  // theme is being migrated.
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   const Extension* extension = service ?
@@ -415,6 +510,15 @@ void ThemeService::MigrateTheme() {
   }
 }
 
+void ThemeService::SwapThemeSupplier(
+    scoped_refptr<CustomThemeSupplier> theme_supplier) {
+  if (theme_supplier_.get())
+    theme_supplier_->StopUsingTheme();
+  theme_supplier_ = theme_supplier;
+  if (theme_supplier_.get())
+    theme_supplier_->StartUsingTheme();
+}
+
 void ThemeService::SavePackName(const base::FilePath& pack_path) {
   profile_->GetPrefs()->SetFilePath(
       prefs::kCurrentThemePackFilename, pack_path);
@@ -471,7 +575,7 @@ void ThemeService::OnInfobarDestroyed() {
   number_of_infobars_--;
 
   if (number_of_infobars_ == 0)
-    RemoveUnusedThemes();
+    RemoveUnusedThemes(false);
 }
 
 ThemeSyncableService* ThemeService::GetThemeSyncableService() const {