Remove NOTIFICATION_EXTENSION_ENABLED.

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 3a0161e9881e235d2e307ff2307da9b772a3538d..dab060c65898b52bb8ba43be2bcefaacdda5bd95 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -49,6 +49,7 @@
 #include "ui/native_theme/native_theme.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "base/scoped_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 #endif
 
@@ -160,17 +161,17 @@ base::RefCountedMemory* ThemeService::BrowserThemeProvider::GetRawData(
 class ThemeService::ThemeObserver
     : public extensions::ExtensionRegistryObserver {
  public:
-  explicit ThemeObserver(ThemeService* service) : theme_service_(service) {
-    extensions::ExtensionRegistry::Get(theme_service_->profile_)
-        ->AddObserver(this);
+  explicit ThemeObserver(ThemeService* service)
+      : theme_service_(service), extension_registry_observer_(this) {
+    extension_registry_observer_.Add(
+        extensions::ExtensionRegistry::Get(theme_service_->profile_));
   }
 
   ~ThemeObserver() override {
-    extensions::ExtensionRegistry::Get(theme_service_->profile_)
-        ->RemoveObserver(this);
   }
 
  private:
+  // extensions::ExtensionRegistryObserver::
   void OnExtensionWillBeInstalled(content::BrowserContext* browser_context,
                                   const extensions::Extension* extension,
                                   bool is_update,
@@ -191,6 +192,12 @@ class ThemeService::ThemeObserver
     theme_service_->installed_pending_load_id_ = kDefaultThemeID;
   }
 
+  void OnExtensionEnabled(content::BrowserContext* browser_context,

[Devlin, 2017/05/17 18:37:48]

Hmm... does this care about enablement?

OnExtensionLoaded(), I think, can be called from three paths:
1. A new extension is added
2. A disabled extension is enabled
3. A terminated extension is reloaded

The logic in OnExtensionWillbeInstalled + OnExtensionLoaded seems to be saying
"if it's a new theme, use it". (1)

The logic here says "if the user re-enabled a disabled theme, use it". (2)

Themes have no process, so can't be terminated. (3 is moot)

So... can we replace all three of these methods with:
OnExtensionLoaded() {
  if (is_theme)
    SetTheme()
}
? 

WDYT?

[lazyboy, 2017/05/17 22:23:25]

Is it possible for a theme to be loaded but it stays disabled for some reason?*
Comment in line 180 seems to be hinting about this, vaguely unfortunately.
In that case we would get OnExtensionLoaded call but won't enable it. (so
setting theme in OnExtensionLoaded would be incorrect).
So if * doesn't happen then the you suggestions sound good!

[Devlin, 2017/05/18 01:34:45]

I don't know how it could be.  If it's disabled, we won't call
OnExtensionLoaded().  There *might* be a way it could be installed disabled, but
then it will never be loaded.

[lazyboy, 2017/05/18 18:10:52]

I missed the comments over installed_pending_load_id_ member, it sounds like
it's trying to handle updating a disabled extension case.
So OnExtensionLoad should be sufficient, I'll run it by themes/ OWNERS. Added
comments here.
but

+                          const extensions::Extension* extension) override {
+    if (extension->is_theme())
+      theme_service_->SetTheme(extension);
+  }
+
   void OnExtensionUnloaded(
       content::BrowserContext* browser_context,
       const extensions::Extension* extension,
@@ -204,6 +211,12 @@ class ThemeService::ThemeObserver
   }
 
   ThemeService* theme_service_;
+
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      extension_registry_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThemeObserver);
 };
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
@@ -258,12 +271,6 @@ void ThemeService::Observe(int type,
                         content::Source<Profile>(profile_));
       OnExtensionServiceReady();
       break;
-    case extensions::NOTIFICATION_EXTENSION_ENABLED: {
-      const Extension* extension = Details<const Extension>(details).ptr();
-      if (extension->is_theme())
-        SetTheme(extension);
-      break;
-    }
     default:
       NOTREACHED();
   }
@@ -791,13 +798,9 @@ void ThemeService::OnExtensionServiceReady() {
   }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  theme_observer_.reset(new ThemeObserver(this));
+  theme_observer_ = base::MakeUnique<ThemeObserver>(this);
 #endif
 
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_ENABLED,
-                 content::Source<Profile>(profile_));
-
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&ThemeService::RemoveUnusedThemes,
                             weak_ptr_factory_.GetWeakPtr(), false),