[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/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/browser_theme_pack.h"
 #include "chrome/browser/themes/custom_theme_supplier.h"
@@ skipping 30 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.
+// Removing unused themes is done after a delay because there is no
+// reason to do it at startup.
+// ExtensionService::GarbageCollectExtensions() does something similar.
+const int kRemoveUnusedThemesStartupDelay = 30;
+
 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() &&
+          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();
+  scoped_ptr<const ExtensionSet> extensions(
+      service->GenerateInstalledExtensionsSet());
+  extensions::ExtensionPrefs* prefs = service->extension_prefs();
   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());
+    const extensions::Extension* extension = *it;
+    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.
+      int disable_reason = prefs->GetDisableReasons(extension->id());
+      if (!prefs->IsExtensionDisabled(extension->id()) ||
+          disable_reason == Extension::DISABLE_USER_ACTION) {
+        remove_list.push_back((*it)->id());
+      }
     }
   }
+  // TODO: Garbage collect all unused themes. This method misses themes which
+  // are installed but not loaded because they are blacklisted by a management
+  // policy provider.
+
   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,

[Jeffrey Yasskin, 2013/08/06 21:46:01]

In local test runs, I'm getting stack traces like:

#0  scoped_refptr<base::internal::IncomingTaskQueue>::operator-> (this=0x150)
    at ../../base/memory/ref_counted.h:256
#1  0x00007ffff5438caf in base::MessageLoop::PostTask(tracked_objects::Location
const&, base::Callback<void ()> const&) (this=0x0, from_here=..., task=...)
    at ../../base/message_loop/message_loop.cc:268
#2  0x0000000002ec6220 in ThemeService::ClearAllThemeData (this=0x27cd8feae020)
    at ../../chrome/browser/themes/theme_service.cc:395
#3  0x0000000003a57523 in GtkThemeService::ClearAllThemeData
(this=0x27cd8feae020)
    at ../../chrome/browser/ui/gtk/gtk_theme_service.cc:637
#4  0x0000000003a55bac in GtkThemeService::SetNativeTheme (this=0x27cd8feae020)
    at ../../chrome/browser/ui/gtk/gtk_theme_service.cc:364
#5  0x0000000002ec62f2 in ThemeService::LoadThemePrefs (this=0x27cd8feae020)
    at ../../chrome/browser/themes/theme_service.cc:408
#6  0x0000000003a57552 in GtkThemeService::LoadThemePrefs (this=0x27cd8feae020)
    at ../../chrome/browser/ui/gtk/gtk_theme_service.cc:647
#7  0x0000000002ec4134 in ThemeService::Init (this=0x27cd8feae020,
profile=0x27cd8fc51630)
    at ../../chrome/browser/themes/theme_service.cc:100
#8  0x0000000003a55024 in GtkThemeService::Init (this=0x27cd8feae020,
profile=0x27cd8fc51630)
    at ../../chrome/browser/ui/gtk/gtk_theme_service.cc:300
#9  0x0000000002ec84db in ThemeServiceFactory::BuildServiceInstanceFor
(this=0x27cd8ff7f260, 
    profile=0x27cd8fc51630) at
../../chrome/browser/themes/theme_service_factory.cc:64
#10 0x00007ffff7f4fb7a in
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext (
    this=0x27cd8ff7f260, context=0x27cd8fc51630, create=true)
    at
../../components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:84
#11 0x00007ffff7f50072 in BrowserContextKeyedServiceFactory::CreateServiceNow (
    this=0x27cd8ff7f260, context=0x27cd8fc51630)
    at
../../components/browser_context_keyed_service/browser_context_keyed_service_factory.cc:129
#12 0x00007ffff7f4c033 in
BrowserContextDependencyManager::CreateBrowserContextServices (
    this=0x27cd8ff6e0f0, context=0x27cd8fc51630, is_testing_context=true)
    at
../../components/browser_context_keyed_service/browser_context_dependency_manager.cc:77
#13 0x0000000003757eba in TestingProfile::Init (this=0x27cd8fc51630)
    at ../../chrome/test/base/testing_profile.cc:312
#14 0x0000000003757989 in TestingProfile::TestingProfile (this=0x27cd8fc51630)
    at ../../chrome/test/base/testing_profile.cc:179
#15 0x0000000000512edf in
apps::ShellWindowGeometryCacheTest::ShellWindowGeometryCacheTest (
    this=0x27cd8fc51620) at
../../apps/shell_window_geometry_cache_unittest.cc:31
#16 0x000000000051489f in
apps::ShellWindowGeometryCacheTest_GetGeometryEmptyStore_Test::ShellWindowGeometryCacheTest_GetGeometryEmptyStore_Test
(this=0x27cd8fc51620)
    at ../../apps/shell_window_geometry_cache_unittest.cc:99
#17 0x0000000000514875 in
apps::ShellWindowGeometryCacheTest_GetGeometryEmptyStore_Test::ShellWindowGeometryCacheTest_GetGeometryEmptyStore_Test
(this=0x27cd8fc51620)
    at ../../apps/shell_window_geometry_cache_unittest.cc:99


Which look to be caused by a TestingProfile getting created before the
MessageLoop. It seems like my execution is taking the !managed &&
ShouldInitWithNativeTheme path in ThemeService::LoadThemePrefs, which the bots
and kalman's local build aren't taking.

+      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_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));
 }
 
 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 35 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();
 }