Remove deprecated extension notification from theme_service

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 7b20d395050affc0ef04d13c678e325166625070..295cf8600cc5d15a6bafcc8ad56f8bede66e4f15 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -96,12 +96,57 @@ bool IsColorGrayscale(SkColor color) {
 
 }  // namespace
 
+#if defined(ENABLE_EXTENSIONS)
+class ThemeService::ExtensionObserver
+    : public extensions::ExtensionRegistryObserver {
+ public:
+  explicit ExtensionObserver(ThemeService& service) : theme_service_(service) {}
+  ~ExtensionObserver() override {}
+  void OnExtensionWillBeInstalled(content::BrowserContext* browser_context,
+                                  const extensions::Extension* extension,
+                                  bool is_update,
+                                  bool from_ephemeral,
+                                  const std::string& old_name) override {
+    if (extension->is_theme()) {
+      // The theme may be initially disabled. Wait till it is loaded (if ever).
+      theme_service_.installed_pending_load_id_ = extension->id();
+    }
+  }
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const extensions::Extension* extension) override {
+    if (extension->is_theme() &&
+        theme_service_.installed_pending_load_id_ != kDefaultThemeID &&
+        theme_service_.installed_pending_load_id_ == extension->id()) {
+      theme_service_.SetTheme(extension);
+    }
+    theme_service_.installed_pending_load_id_ = kDefaultThemeID;
+  }
+  void OnExtensionUnloaded(
+      content::BrowserContext* browser_context,
+      const extensions::Extension* extension,
+      extensions::UnloadedExtensionInfo::Reason reason) override {
+    if (reason != extensions::UnloadedExtensionInfo::REASON_UPDATE &&
+        reason != extensions::UnloadedExtensionInfo::REASON_LOCK_ALL &&
+        extension->is_theme() &&
+        extension->id() == theme_service_.GetThemeID()) {
+      theme_service_.UseDefaultTheme();
+    }
+  }
+
+ private:
+  ThemeService& theme_service_;

[not at google - send to devlin, 2015/03/12 17:08:33]

nit: usually you would hold onto a pointer, not a reference (and pass in a
pointer, not a reference, above - to imply mutability)

[limasdf, 2015/03/13 03:01:54]

Done.

+};
+#endif  // defined(ENABLE_EXTENSIONS)
+
 ThemeService::ThemeService()
     : ready_(false),
       rb_(ResourceBundle::GetSharedInstance()),
       profile_(NULL),
       installed_pending_load_id_(kDefaultThemeID),
       number_of_infobars_(0),
+#if defined(ENABLE_EXTENSIONS)
+      extension_registry_(NULL),

[not at google - send to devlin, 2015/03/12 17:08:33]

nullptr

[limasdf, 2015/03/13 03:01:54]

Done. including other place as well for the consistency.

+#endif
       weak_ptr_factory_(this) {
 }
 
@@ -114,6 +159,9 @@ void ThemeService::Init(Profile* profile) {
   profile_ = profile;
 
   LoadThemePrefs();
+#if defined(ENABLE_EXTENSIONS)
+  extension_registry_ = extensions::ExtensionRegistry::Get(profile_);
+#endif
 
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED,
@@ -257,6 +305,12 @@ base::RefCountedMemory* ThemeService::GetRawData(
   return data;
 }
 
+void ThemeService::Shutdown() {
+#if defined(ENABLE_EXTENSIONS)
+  extension_registry_->RemoveObserver(extension_observer_.get());

[not at google - send to devlin, 2015/03/12 17:08:33]

(see below - you might as well destroy the observer as well)

[limasdf, 2015/03/13 03:01:54]

Done.

+#endif
+}
+
 void ThemeService::Observe(int type,
                            const content::NotificationSource& source,
                            const content::NotificationDetails& details) {
@@ -268,40 +322,14 @@ void ThemeService::Observe(int type,
                         content::Source<Profile>(profile_));
       OnExtensionServiceReady();
       break;
-    case extensions::NOTIFICATION_EXTENSION_WILL_BE_INSTALLED_DEPRECATED: {
-      // 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 extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED: {
-      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 extensions::NOTIFICATION_EXTENSION_ENABLED: {
       const Extension* extension = Details<const Extension>(details).ptr();
       if (extension->is_theme())
         SetTheme(extension);
       break;
     }
-    case extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED: {
-      Details<const UnloadedExtensionInfo> unloaded_details(details);
-      if (unloaded_details->reason != UnloadedExtensionInfo::REASON_UPDATE &&
-          unloaded_details->reason != UnloadedExtensionInfo::REASON_LOCK_ALL &&
-          unloaded_details->extension->is_theme() &&
-          unloaded_details->extension->id() == GetThemeID()) {
-        UseDefaultTheme();
-      }
-      break;
-    }
+    default:
+      NOTREACHED();
   }
 }
 
@@ -529,19 +557,14 @@ void ThemeService::OnExtensionServiceReady() {
     NotifyThemeChanged();
   }
 
-  registrar_.Add(
-      this,
-      extensions::NOTIFICATION_EXTENSION_WILL_BE_INSTALLED_DEPRECATED,
-      content::Source<Profile>(profile_));
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
-                 content::Source<Profile>(profile_));
+#if defined(ENABLE_EXTENSIONS)
+  extension_observer_.reset(new ExtensionObserver(*this));
+  extension_registry_->AddObserver(extension_observer_.get());

[not at google - send to devlin, 2015/03/12 17:08:33]

If you make ExtensionObserver add/remove itself as an Observer of
ExtensionRegistry itself, then all you need is the reset(...) here and the
reset() above.

That would also make it more natural to remove the awkwardness of holding onto
the ExtensionRegistry* itself, and let you remove even more #ifdefs.

[limasdf, 2015/03/13 03:01:54]

Done.
Yes. Exactly. 

+#endif
+
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSION_ENABLED,
                  content::Source<Profile>(profile_));
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED,
-                 content::Source<Profile>(profile_));
 
   base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
       base::Bind(&ThemeService::RemoveUnusedThemes,