Make ThemeService use the ExtensionSystem's AsyncEvent

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 5b8fa92d442938010d78b2162a86462221943a80..29450003ffdbb208395ad6ae755628af4de8207c 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -79,7 +79,8 @@ ThemeService::ThemeService()
     : rb_(ResourceBundle::GetSharedInstance()),
       profile_(NULL),
       ready_(false),
-      number_of_infobars_(0) {
+      number_of_infobars_(0),
+      weak_ptr_factory_(this) {
 }
 
 ThemeService::~ThemeService() {
@@ -92,12 +93,6 @@ void ThemeService::Init(Profile* profile) {
 
   LoadThemePrefs();
 
-  if (!ready_) {
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_EXTENSIONS_READY,
-                   content::Source<Profile>(profile_));
-  }
-
   theme_syncable_service_.reset(new ThemeSyncableService(profile_, this));
 }
 
@@ -205,21 +200,6 @@ base::RefCountedMemory* ThemeService::GetRawData(
   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();
-}
-
 void ThemeService::SetTheme(const Extension* extension) {
   // Clear our image cache.
   FreePlatformCaches();
@@ -328,12 +308,10 @@ void ThemeService::LoadThemePrefs() {
   } 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();
-    }
+    extensions::ExtensionSystem::Get(profile_)->ready()
+        .ThenRun(FROM_HERE,
+                 base::Bind(&ThemeService::FinishLoadingAfterExtensionsReady,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
@@ -361,6 +339,15 @@ void ThemeService::FreePlatformCaches() {
 }
 #endif
 
+void ThemeService::FinishLoadingAfterExtensionsReady() {
+  MigrateTheme();
+  set_ready();
+
+  // Send notification in case anyone requested data and cached it when the
+  // theme service was not ready yet.
+  NotifyThemeChanged();

[pkotwicz, 2013/05/16 18:18:35]

Delaying when we call NotifyThemeChanged() gives a lot more code the opportunity
to register to listen to NOTIFICATION_BROWSER_THEME_CHANGED. This subsequently
makes the call to NotifyThemeChanged() significantly more expensive on Windows.
We only go through this code path after the user has upgraded to a new version
of Chrome so I am not sure how much this matters.
Otherwise, the code looks good

[Jeffrey Yasskin, 2013/05/16 23:28:18]

In the long run, I think we can fix this by making ThemeService::ready_ a
OneShotEvent too, and doing the same cleanup on the users of
NOTIFICATION_BROWSER_THEME_CHANGED. In the short run ...

We could in theory add a ThenRun(..., INLINE_OK) overload, which allows running
the callback without yielding back to the MessageLoop, which I think would
eliminate the extra listeners on NOTIFICATION_BROWSER_THEME_CHANGED in the case
that the ExtensionsSystem is ready when the ThemeService is constructed. (*Is*
the ExtensionSystem generally ready when the ThemeService is constructed?)
However, we're hoping to avoid that INLINE_OK flag because of past bad
experiences with calls that could either run inline or yield.

I don't immediately see a good way to call NotifyThemeChanged only if the task
is delayed, without going back to the duplicated-code situation.

Do you think the extra NotifyThemeChanged cost is enough that I should rethink
this change, or is it acceptable in order to eliminate the duplicated code?
(and, in the long run, all uses of EXTENSIONS_READY)

+}
+
 void ThemeService::MigrateTheme() {
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();