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

chrome/browser/themes/theme_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/themes/theme_service.h"
 
 #include "base/bind.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/prefs/pref_service.h"
 #include "base/sequenced_task_runner.h"
@@ skipping 39 matching lines @@
 const char* ThemeService::kDefaultThemeID = "";
 
 namespace {
 
 // The default theme if we've gone to the theme gallery and installed the
 // "Default" theme. We have to detect this case specifically. (By the time we
 // realize we've installed the default theme, we already have an extension
 // 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);
 }
 
 SkColor IncreaseLightness(SkColor color, double percent) {
   color_utils::HSL result;
   color_utils::SkColorToHSL(color, &result);
   result.l += (1 - result.l) * percent;
   return color_utils::HSLToSkColor(result, SkColorGetA(color));
 }
 
 // Writes the theme pack to disk on a separate thread.
 void WritePackToDiskCallback(BrowserThemePack* pack,
                              const base::FilePath& path) {
   if (!pack->WriteToDisk(path))
     NOTREACHED() << "Could not write theme pack to disk";
 }
 
 }  // namespace
 
 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() {
   FreePlatformCaches();
 }
 
 void ThemeService::Init(Profile* profile) {
   DCHECK(CalledOnValidThread());
   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));
 }
 
 gfx::Image ThemeService::GetImageNamed(int id) const {
   DCHECK(CalledOnValidThread());
 
   gfx::Image image;
   if (theme_supplier_.get())
     image = theme_supplier_->GetImageNamed(id);
@@ skipping 85 matching lines @@
     data = theme_supplier_->GetRawData(id, scale_factor);
   if (!data)
     data = rb_.LoadDataResourceBytesForScale(id, ui::SCALE_FACTOR_100P);
 
   return data;
 }
 
 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) {
+  DCHECK(extension->is_theme());
+  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();
 
-  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));
-  }
-
   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(
     scoped_refptr<CustomThemeSupplier> theme_supplier) {
   ClearAllThemeData();
   SwapThemeSupplier(theme_supplier);
   NotifyThemeChanged();
 }
 
 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)
     return;
   std::string current_theme = GetThemeID();
   std::vector<std::string> remove_list;
   const ExtensionSet* extensions = service->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());
     }
   }
+  // 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);
 }
 
 void ThemeService::UseDefaultTheme() {
   if (ready_)
     content::RecordAction(UserMetricsAction("Themes_Reset"));
   if (IsManagedUser()) {
     SetManagedUserTheme();
     return;
@@ skipping 34 matching lines @@
 
 void ThemeService::ClearAllThemeData() {
   SwapThemeSupplier(NULL);
 
   // Clear our image cache.
   FreePlatformCaches();
 
   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() {
   PrefService* prefs = profile_->GetPrefs();
 
   std::string current_id = GetThemeID();
   if (current_id == kDefaultThemeID) {
     // Managed users have a different default theme.
     if (IsManagedUser())
       SetManagedUserTheme();
@@ skipping 10 matching lines @@
   // If we don't have a file pack, we're updating from an old version.
   base::FilePath path = prefs->GetFilePath(prefs::kCurrentThemePackFilename);
   if (path != base::FilePath()) {
     SwapThemeSupplier(BrowserThemePack::BuildFromDataPack(path, current_id));
     loaded_pack = theme_supplier_.get() != NULL;
   }
 
   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() {
   if (!ready_)
     return;
 
   DVLOG(1) << "Sending BROWSER_THEME_CHANGED";
   // Redraw!
   content::NotificationService* service =
       content::NotificationService::current();
   service->Notify(chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
                   content::Source<ThemeService>(this),
                   content::NotificationService::NoDetails());
 #if defined(OS_MACOSX)
   NotifyPlatformThemeChanged();
 #endif  // OS_MACOSX
 
   // Notify sync that theme has changed.
   if (theme_syncable_service_.get()) {
     theme_syncable_service_->OnThemeChange();
   }
 }
 
 #if defined(OS_WIN) || defined(USE_AURA)
 void ThemeService::FreePlatformCaches() {
   // Views (Skia) has no platform image cache to clear.
 }
 #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 ?
       service->GetExtensionById(GetThemeID(), false) : NULL;
   if (extension) {
     DLOG(ERROR) << "Migrating theme";
     BuildFromExtension(extension);
     content::RecordAction(UserMetricsAction("Themes.Migrated"));
   } else {
     DLOG(ERROR) << "Theme is mysteriously gone.";
     ClearAllThemeData();
     content::RecordAction(UserMetricsAction("Themes.Gone"));
   }
 }
 
+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);
 }
 
 void ThemeService::SaveThemeID(const std::string& id) {
   profile_->GetPrefs()->SetString(prefs::kCurrentThemeID, id);
 }
 
 void ThemeService::BuildFromExtension(const Extension* extension) {
@@ skipping 36 matching lines @@
 }
 
 void ThemeService::OnInfobarDisplayed() {
   number_of_infobars_++;
 }
 
 void ThemeService::OnInfobarDestroyed() {
   number_of_infobars_--;
 
   if (number_of_infobars_ == 0)
-    RemoveUnusedThemes();
+    RemoveUnusedThemes(false);
 }
 
 ThemeSyncableService* ThemeService::GetThemeSyncableService() const {
   return theme_syncable_service_.get();
 }