Remove NOTIFICATION_EXTENSION_ENABLED.

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 <stddef.h>
 
 #include <algorithm>
 
@@ skipping 31 matching lines @@
 #include "extensions/common/extension_set.h"
 #include "extensions/features/features.h"
 #include "ui/base/layout.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/native_theme/common_theme.h"
 #include "ui/native_theme/native_theme.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "base/scoped_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 #endif
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
 #include "chrome/browser/supervised_user/supervised_user_theme.h"
 #endif
 
 using base::UserMetricsAction;
 using content::BrowserThread;
 using extensions::Extension;
@@ skipping 91 matching lines @@
   return theme_service_.GetRawData(id, scale_factor);
 }
 
 
 // ThemeService::ThemeObserver ------------------------------------------------
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 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,
                                   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 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,
       extensions::UnloadedExtensionReason reason) override {
     if (reason != extensions::UnloadedExtensionReason::UPDATE &&
         reason != extensions::UnloadedExtensionReason::LOCK_ALL &&
         extension->is_theme() &&
         extension->id() == theme_service_->GetThemeID()) {
       theme_service_->UseDefaultTheme();
     }
   }
 
   ThemeService* theme_service_;
+
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      extension_registry_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThemeObserver);
 };
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
 
 // ThemeService ---------------------------------------------------------------
 
 // The default theme if we haven't installed a theme yet or if we've clicked
 // the "Use Classic" button.
 const char ThemeService::kDefaultThemeID[] = "";
 
@@ skipping 34 matching lines @@
                            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_ENABLED: {
-      const Extension* extension = Details<const Extension>(details).ptr();
-      if (extension->is_theme())
-        SetTheme(extension);
-      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())) {
@@ skipping 507 matching lines @@
     // 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)
-  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),
       base::TimeDelta::FromSeconds(kRemoveUnusedThemesStartupDelay));
 }
 
 void ThemeService::MigrateTheme() {
   // TODO(erg): We need to pop up a dialog informing the user that their
   // theme is being migrated.
   ExtensionService* service =
@@ skipping 57 matching lines @@
 
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
 bool ThemeService::IsSupervisedUser() const {
   return profile_->IsSupervised();
 }
 
 void ThemeService::SetSupervisedUserTheme() {
   SetCustomDefaultTheme(new SupervisedUserTheme);
 }
 #endif