Remove deprecated extension notification from theme_service

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 <algorithm>
 
 #include "base/bind.h"
 #include "base/memory/ref_counted_memory.h"
@@ skipping 78 matching lines @@
   const int kChannelTolerance = 9;
   int r = SkColorGetR(color);
   int g = SkColorGetG(color);
   int b = SkColorGetB(color);
   int range = std::max(r, std::max(g, b)) - std::min(r, std::min(g, b));
   return range < kChannelTolerance;
 }
 
 }  // 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) {
 }
 
 ThemeService::~ThemeService() {
   FreePlatformCaches();
 }
 
 void ThemeService::Init(Profile* profile) {
   DCHECK(CalledOnValidThread());
   profile_ = profile;
 
   LoadThemePrefs();
+#if defined(ENABLE_EXTENSIONS)
+  extension_registry_ = extensions::ExtensionRegistry::Get(profile_);
+#endif
 
   registrar_.Add(this,
                  extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED,
                  content::Source<Profile>(profile_));
 
   theme_syncable_service_.reset(new ThemeSyncableService(profile_, this));
 }
 
 gfx::Image ThemeService::GetImageNamed(int id) const {
   DCHECK(CalledOnValidThread());
@@ skipping 123 matching lines @@
 
   base::RefCountedMemory* data = NULL;
   if (theme_supplier_.get())
     data = theme_supplier_->GetRawData(id, scale_factor);
   if (!data)
     data = rb_.LoadDataResourceBytesForScale(id, ui::SCALE_FACTOR_100P);
 
   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) {
   using content::Details;
   switch (type) {
     case extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED:
       registrar_.Remove(this,
                         extensions::NOTIFICATION_EXTENSIONS_READY_DEPRECATED,
                         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();
   }
 }
 
 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.
@@ skipping 207 matching lines @@
     // 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,
-      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,
                  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
@@ skipping 74 matching lines @@
 void ThemeService::OnInfobarDestroyed() {
   number_of_infobars_--;
 
   if (number_of_infobars_ == 0)
     RemoveUnusedThemes(false);
 }
 
 ThemeSyncableService* ThemeService::GetThemeSyncableService() const {
   return theme_syncable_service_.get();
 }