Unpack theme data from extensions off of UI thread.

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 6a9d0e1e40a102972fba8dd3e20467802ff9fe0a..29ce4a01dd41648af47f1bda4aa70a02e0c2c9de 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -17,10 +17,12 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/theme_installed_infobar_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/browser_theme_pack.h"
 #include "chrome/browser/themes/custom_theme_supplier.h"
@@ -262,7 +264,7 @@ void ThemeService::Observe(int type,
     case extensions::NOTIFICATION_EXTENSION_ENABLED: {
       const Extension* extension = Details<const Extension>(details).ptr();
       if (extension->is_theme())
-        SetTheme(extension);
+        DoSetTheme(extension, true);
       break;
     }
     default:
@@ -271,40 +273,18 @@ void ThemeService::Observe(int type,
 }
 
 void ThemeService::SetTheme(const Extension* extension) {
+  DoSetTheme(extension, false);
+}
+
+void ThemeService::RevertToTheme(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();
-
-  BuildFromExtension(extension);
-  SaveThemeID(extension->id());
-
-  NotifyThemeChanged();
-  base::RecordAction(UserMetricsAction("Themes_Installed"));
-
-  if (previous_theme_id != kDefaultThemeID &&
-      previous_theme_id != extension->id() &&
-      service->GetInstalledExtension(previous_theme_id)) {
-    // Do not disable the previous theme if it is already uninstalled. Sending
-    // NOTIFICATION_BROWSER_THEME_CHANGED causes the previous theme to be
-    // uninstalled when the notification causes the remaining infobar to close
-    // and does not open any new infobars. See crbug.com/468280.
-
-    // Disable the old theme.
-    service->DisableExtension(previous_theme_id,
-                              extensions::Extension::DISABLE_USER_ACTION);
-  }
+  DCHECK(!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().
 }
 
 void ThemeService::UseDefaultTheme() {
@@ -349,8 +329,10 @@ void ThemeService::OnInfobarDisplayed() {
 void ThemeService::OnInfobarDestroyed() {
   number_of_infobars_--;
 
-  if (number_of_infobars_ == 0)
+  if (number_of_infobars_ == 0 &&
+      !build_extension_task_tracker_.HasTrackedTasks()) {
     RemoveUnusedThemes(false);
+  }
 }
 
 void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
@@ -375,8 +357,8 @@ void ThemeService::RemoveUnusedThemes(bool ignore_infobars) {
   for (extensions::ExtensionSet::const_iterator it = extensions->begin();
        it != extensions->end(); ++it) {
     const extensions::Extension* extension = it->get();
-    if (extension->is_theme() &&
-        extension->id() != current_theme) {
+    if (extension->is_theme() && extension->id() != current_theme &&
+        extension->id() != building_extension_id_) {
       // 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.
@@ -578,7 +560,7 @@ void ThemeService::LoadThemePrefs() {
 
   // 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()) {
+  if (!path.empty()) {
     path = path.Append(chrome::kThemePackFilename);
     SwapThemeSupplier(BrowserThemePack::BuildFromDataPack(path, current_id));
     if (theme_supplier_)
@@ -685,6 +667,15 @@ SkColor ThemeService::GetSeparatorColor(SkColor tab_color,
   return SkColorSetA(separator_color, alpha);
 }
 
+void ThemeService::DoSetTheme(const Extension* extension,
+                              bool suppress_infobar) {
+  DCHECK(extension->is_theme());
+  DCHECK(extensions::ExtensionSystem::Get(profile_)
+             ->extension_service()
+             ->IsExtensionEnabled(extension->id()));
+  BuildFromExtension(extension, suppress_infobar);
+}
+
 gfx::ImageSkia* ThemeService::GetImageSkiaNamed(int id, bool incognito) const {
   gfx::Image image = GetImageNamed(id, incognito);
   if (image.IsEmpty())
@@ -785,10 +776,6 @@ void ThemeService::OnExtensionServiceReady() {
     // 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();
   }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -806,15 +793,18 @@ void ThemeService::OnExtensionServiceReady() {
 }
 
 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) : nullptr;
   if (extension) {
     DLOG(ERROR) << "Migrating theme";
-    BuildFromExtension(extension);
+    // Theme migration is done on the UI thread. Blocking the UI from appearing
+    // until it's ready is deemed better than showing a blip of the default
+    // theme.
+    scoped_refptr<BrowserThemePack> pack(new BrowserThemePack);
+    BrowserThemePack::BuildFromExtension(extension, pack);
+    OnThemeBuiltFromExtension(extension->id(), pack, true);
     base::RecordAction(UserMetricsAction("Themes.Migrated"));
   } else {
     DLOG(ERROR) << "Theme is mysteriously gone.";
@@ -841,10 +831,26 @@ void ThemeService::SaveThemeID(const std::string& id) {
   profile_->GetPrefs()->SetString(prefs::kCurrentThemeID, id);
 }
 
-void ThemeService::BuildFromExtension(const Extension* extension) {
-  scoped_refptr<BrowserThemePack> pack(
-      BrowserThemePack::BuildFromExtension(extension));
-  if (!pack.get()) {
+void ThemeService::BuildFromExtension(const Extension* extension,
+                                      bool suppress_infobar) {
+  build_extension_task_tracker_.TryCancelAll();
+  building_extension_id_ = extension->id();
+  scoped_refptr<BrowserThemePack> pack(new BrowserThemePack);
+  auto task_runner = base::CreateTaskRunnerWithTraits(
+      {base::MayBlock(), base::TaskPriority::USER_BLOCKING});
+  build_extension_task_tracker_.PostTaskAndReply(
+      task_runner.get(), FROM_HERE,
+      base::Bind(&BrowserThemePack::BuildFromExtension, extension, pack),
+      base::Bind(&ThemeService::OnThemeBuiltFromExtension,
+                 weak_ptr_factory_.GetWeakPtr(), extension->id(), pack,
+                 suppress_infobar));
+}
+
+void ThemeService::OnThemeBuiltFromExtension(
+    const extensions::ExtensionId& extension_id,
+    scoped_refptr<BrowserThemePack> pack,
+    bool suppress_infobar) {
+  if (!pack->is_valid()) {
     // TODO(erg): We've failed to install the theme; perhaps we should tell the
     // user? http://crbug.com/34780
     LOG(ERROR) << "Could not load theme.";
@@ -855,6 +861,11 @@ void ThemeService::BuildFromExtension(const Extension* extension) {
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   if (!service)
     return;
+  const Extension* extension = extensions::ExtensionRegistry::Get(profile_)
+                                   ->enabled_extensions()
+                                   .GetByID(extension_id);
+  if (!extension)
+    return;
 
   // Write the packed file to disk.
   service->GetFileTaskRunner()->PostTask(
@@ -865,6 +876,46 @@ void ThemeService::BuildFromExtension(const Extension* extension) {
   // LoadThemePrefs().
   SavePackName(extension->path());
   SwapThemeSupplier(pack);
+
+  if (extension->id() == GetThemeID()) {
+    // Same old theme, but the theme has changed (migrated).
+    NotifyThemeChanged();
+    return;
+  }
+
+  const std::string previous_theme_id = GetThemeID();
+  const bool previous_using_system_theme = UsingSystemTheme();
+
+  // Clear our image cache.
+  FreePlatformCaches();
+  SaveThemeID(extension->id());
+  NotifyThemeChanged();

[pkotwicz, 2017/06/01 03:21:08]

Can this be simplified as:

const std::string previous_theme_id = GetThemeID();
const bool previous_using_system_theme = UsingSystemTheme();

// Clear our image cache.
FreePlatformCaches();
SaveThemeID(extension->id());
NotifyThemeChanged();

if (previous_theme_id != extension_id)
  return;

I am unsure whether your current code works for themes which auto update
themselves using the mechanism in
https://developer.chrome.com/extensions/autoupdate I am unsure what happens (if
anything) when a theme is updated via the Web Store

[Evan Stade, 2017/06/01 17:43:02]

seems safe to make that change. FreePlatformCaches is a no-op on !mac and
SaveThemeID with the same ID will be one as well. Seems like mac might want
FreePlatformCaches.

+
+  base::RecordAction(UserMetricsAction("Themes_Installed"));
+
+  bool previous_theme_available =
+      previous_theme_id == kDefaultThemeID || previous_using_system_theme;

[pkotwicz, 2017/06/01 03:21:08]

The system theme uses kDefaultThemeID

[Evan Stade, 2017/06/01 17:43:02]

pretty sure this is necessary for GTK to work

[pkotwicz, 2017/06/01 19:09:05]

That's untrue.

ThemeServiceAuraX11::UseSystemTheme() invokes the following call chain

ThemeServiceAuraX11::UseSystemTheme() -> ThemeService::SetCustomDefaultTheme()
-> ThemeService::ClearAllThemeData() -> SaveThemeID(kDefaultThemeID)

[Evan Stade, 2017/06/01 23:58:54]

ok, thanks for pointing this out.

+  if (previous_theme_id != kDefaultThemeID &&

[pkotwicz, 2017/06/01 03:21:08]

Can

previous_theme_id != kDefaultThemeId

be simplified to

!previous_theme_available

[Evan Stade, 2017/06/01 17:43:02]

perhaps, but that reads really strangely IMO, because in this case the previous
theme may actually be available.

[pkotwicz, 2017/06/01 19:09:05]

Fair enough

+      previous_theme_id != extension_id &&

[pkotwicz, 2017/06/01 03:21:09]

I think that the following is always true

previous_theme_id != extension_id

due to early exit on line 883

[Evan Stade, 2017/06/01 17:43:02]

Done.

+      service->GetInstalledExtension(previous_theme_id)) {
+    // Do not disable the previous theme if it is already uninstalled. Sending
+    // NOTIFICATION_BROWSER_THEME_CHANGED causes the previous theme to be
+    // uninstalled when the notification causes the remaining infobar to close
+    // and does not open any new infobars. See crbug.com/468280.
+
+    // Disable the old theme.
+    service->DisableExtension(previous_theme_id,
+                              extensions::Extension::DISABLE_USER_ACTION);
+
+    previous_theme_available = true;
+  }
+
+  // Offer to revert to the old theme.
+  if (previous_theme_available && !suppress_infobar) {
+    ThemeInstalledInfoBarDelegate::Create(
+        extension, profile_, previous_theme_id, previous_using_system_theme);
+  }
+  building_extension_id_.clear();
 }
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)