Revert changes to have ExtensionService notify ThemeService directly

This reverts r159875 and r170640.  It cause a performance regression
that's not easy to fix, as detailed in the bug.

BUG=163706
TBR=asargent@chromium.org,erg@chromium.org,pkotwicz@chromium.org

Review URL: https://codereview.chromium.org/11416324

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170824 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 102 insertions, 121 deletions

diff --git a/chrome/browser/extensions/extension_install_ui_browsertest.cc b/chrome/browser/extensions/extension_install_ui_browsertest.cc
index 3bd833e..66795de 100644
--- a/chrome/browser/extensions/extension_install_ui_browsertest.cc
+++ b/chrome/browser/extensions/extension_install_ui_browsertest.cc
@@ -50,12 +50,7 @@ class ExtensionInstallUIBrowserTest : public ExtensionBrowserTest {
   }
 
   const Extension* GetTheme() const {
-    Profile* profile = browser()->profile();
-    std::string theme_id = ThemeService::GetThemeIDForProfile(profile);
-    return
-        (theme_id == ThemeService::kDefaultThemeID) ?
-        NULL :
-        profile->GetExtensionService()->GetExtensionById(theme_id, false);
+    return ThemeServiceFactory::GetThemeForProfile(browser()->profile());
   }
 };
 
diff --git a/chrome/browser/extensions/extension_install_ui_default.cc b/chrome/browser/extensions/extension_install_ui_default.cc
index 91965d4..a058ea6 100644
--- a/chrome/browser/extensions/extension_install_ui_default.cc
+++ b/chrome/browser/extensions/extension_install_ui_default.cc
@@ -100,7 +100,10 @@ ExtensionInstallUIDefault::ExtensionInstallUIDefault(Profile* profile)
   // |profile_| can be NULL during tests.
   if (profile_) {
     // Remember the current theme in case the user presses undo.
-    previous_theme_id_ = ThemeService::GetThemeIDForProfile(profile);
+    const Extension* previous_theme =
+        ThemeServiceFactory::GetThemeForProfile(profile);
+    if (previous_theme)
+      previous_theme_id_ = previous_theme->id();
     previous_using_native_theme_ =
         ThemeServiceFactory::GetForProfile(profile)->UsingNativeTheme();
   }
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 9708733..a10372e 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -80,6 +80,8 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/themes/theme_service.h"
+#include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/webui/chrome_url_data_manager.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
@@ -116,11 +118,6 @@
 #include "webkit/database/database_tracker.h"
 #include "webkit/database/database_util.h"
 
-#if defined(ENABLE_THEMES)
-#include "chrome/browser/themes/theme_service.h"
-#include "chrome/browser/themes/theme_service_factory.h"
-#endif
-
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/extensions/file_browser_event_router.h"
@@ -1034,9 +1031,9 @@ void ExtensionService::NotifyExtensionLoaded(const Extension* extension) {
   // extension.
   system_->RegisterExtensionWithRequestContexts(extension);
 
+  // Tell renderers about the new extension, unless it's a theme (renderers
+  // don't need to know about themes).
   if (!extension->is_theme()) {
-    // Tell renderers about non-theme extensions (renderers don't need
-    // to know about themes).
     for (content::RenderProcessHost::iterator i(
             content::RenderProcessHost::AllHostsIterator());
          !i.IsAtEnd(); i.Advance()) {
@@ -2029,7 +2026,7 @@ void ExtensionService::GarbageCollectExtensions() {
   // defensive; in the future, we may call GarbageCollectExtensions()
   // from somewhere other than Init() (e.g., in a timer).
   if (profile_) {
-    ThemeService::RemoveUnusedThemesForProfile(profile_);
+    ThemeServiceFactory::GetForProfile(profile_)->RemoveUnusedThemes();
   }
 #endif
 }
@@ -2110,13 +2107,6 @@ void ExtensionService::AddExtension(const Extension* extension) {
   SyncExtensionChangeIfNeeded(*extension);
   NotifyExtensionLoaded(extension);
   DoPostLoadTasks(extension);
-
-#if defined(ENABLE_THEMES)
-  if (extension->is_theme()) {
-    // Notify the ThemeService about the newly-installed theme.
-    ThemeServiceFactory::SetThemeForProfile(profile_, extension);
-  }
-#endif
 }
 
 void ExtensionService::AddComponentExtension(const Extension* extension) {
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
index 4924c9c..4dfe640 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -224,6 +224,14 @@ void ThemeService::Init(Profile* profile) {
   DCHECK(CalledOnValidThread());
   profile_ = profile;
 
+  // Listen to EXTENSION_LOADED instead of EXTENSION_INSTALLED because
+  // the extension cannot yet be found via GetExtensionById() if it is
+  // installed but not loaded (which may confuse listeners to
+  // BROWSER_THEME_CHANGED).
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_LOADED,
+                 content::Source<Profile>(profile_));
+
   LoadThemePrefs();
 
   theme_syncable_service_.reset(new ThemeSyncableService(profile_, this));
@@ -342,12 +350,31 @@ void ThemeService::SetTheme(const Extension* extension) {
   DCHECK(extension->is_theme());
 
   BuildFromExtension(extension);
-  SaveThemeIDForProfile(profile_, extension->id());
+  SaveThemeID(extension->id());
 
   NotifyThemeChanged();
   content::RecordAction(UserMetricsAction("Themes_Installed"));
 }
 
+void ThemeService::RemoveUnusedThemes() {
+  if (!profile_)
+    return;
+  ExtensionService* service = profile_->GetExtensionService();
+  if (!service)
+    return;
+  std::string current_theme = GetThemeID();
+  std::vector<std::string> remove_list;
+  const ExtensionSet* extensions = service->extensions();
+  for (ExtensionSet::const_iterator it = extensions->begin();
+       it != extensions->end(); ++it) {
+    if ((*it)->is_theme() && (*it)->id() != current_theme) {
+      remove_list.push_back((*it)->id());
+    }
+  }
+  for (size_t i = 0; i < remove_list.size(); ++i)
+    service->UninstallExtension(remove_list[i], false, NULL);
+}
+
 void ThemeService::UseDefaultTheme() {
   ClearAllThemeData();
   NotifyThemeChanged();
@@ -368,50 +395,8 @@ bool ThemeService::UsingNativeTheme() const {
   return UsingDefaultTheme();
 }
 
-void ThemeService::OnInfobarDisplayed() {
-  number_of_infobars_++;
-}
-
-void ThemeService::OnInfobarDestroyed() {
-  number_of_infobars_--;
-
-  if (number_of_infobars_ == 0)
-    RemoveUnusedThemesForProfile(profile_);
-}
-
-// static
-std::string ThemeService::GetThemeIDForProfile(Profile* profile) {
-  return profile->GetPrefs()->GetString(prefs::kCurrentThemeID);
-}
-
-// static
-void ThemeService::SaveThemeIDForProfile(
-    Profile* profile,
-    const std::string& id) {
-  profile->GetPrefs()->SetString(prefs::kCurrentThemeID, id);
-}
-
-// static
-void ThemeService::RemoveUnusedThemesForProfile(Profile* profile) {
-  ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(profile)->extension_service();
-  if (!extension_service)
-    return;
-  std::string current_theme = GetThemeIDForProfile(profile);
-  std::vector<std::string> remove_list;
-  const ExtensionSet* extensions = extension_service->extensions();
-  for (ExtensionSet::const_iterator it = extensions->begin();
-       it != extensions->end(); ++it) {
-    if ((*it)->is_theme() && (*it)->id() != current_theme) {
-      remove_list.push_back((*it)->id());
-    }
-  }
-  for (size_t i = 0; i < remove_list.size(); ++i)
-    extension_service->UninstallExtension(remove_list[i], false, NULL);
-}
-
 std::string ThemeService::GetThemeID() const {
-  return GetThemeIDForProfile(profile_);
+  return profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);
 }
 
 // static
@@ -606,7 +591,7 @@ void ThemeService::ClearAllThemeData() {
   theme_pack_ = NULL;
 
   profile_->GetPrefs()->ClearPref(prefs::kCurrentThemePackFilename);
-  SaveThemeIDForProfile(profile_, kDefaultThemeID);
+  SaveThemeID(kDefaultThemeID);
 }
 
 void ThemeService::LoadThemePrefs() {
@@ -671,11 +656,26 @@ void ThemeService::FreePlatformCaches() {
 }
 #endif
 
+void ThemeService::Observe(int type,
+                           const content::NotificationSource& source,
+                           const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_EXTENSION_LOADED);
+  const Extension* extension = content::Details<const Extension>(details).ptr();
+  if (!extension->is_theme()) {
+    return;
+  }
+  SetTheme(extension);
+}
+
 void ThemeService::SavePackName(const FilePath& pack_path) {
   profile_->GetPrefs()->SetFilePath(
       prefs::kCurrentThemePackFilename, pack_path);
 }
 
+void ThemeService::SaveThemeID(const std::string& id) {
+  profile_->GetPrefs()->SetString(prefs::kCurrentThemeID, id);
+}
+
 void ThemeService::BuildFromExtension(const Extension* extension) {
   scoped_refptr<BrowserThemePack> pack(
       BrowserThemePack::BuildFromExtension(extension));
@@ -701,6 +701,17 @@ void ThemeService::BuildFromExtension(const Extension* extension) {
   theme_pack_ = pack;
 }
 
+void ThemeService::OnInfobarDisplayed() {
+  number_of_infobars_++;
+}
+
+void ThemeService::OnInfobarDestroyed() {
+  number_of_infobars_--;
+
+  if (number_of_infobars_ == 0)
+    RemoveUnusedThemes();
+}
+
 ThemeSyncableService* ThemeService::GetThemeSyncableService() const {
   return theme_syncable_service_.get();
 }
diff --git a/chrome/browser/themes/theme_service.h b/chrome/browser/themes/theme_service.h
index b37a348..b29dde8 100644
--- a/chrome/browser/themes/theme_service.h
+++ b/chrome/browser/themes/theme_service.h
@@ -15,6 +15,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 #include "ui/base/theme_provider.h"
 
 class BrowserThemePack;
@@ -47,6 +49,7 @@ extern "C" NSString* const kBrowserThemeDidChangeNotification;
 #endif  // __OBJC__
 
 class ThemeService : public base::NonThreadSafe,
+                     public content::NotificationObserver,
                      public ProfileKeyedService,
                      public ui::ThemeProvider {
  public:
@@ -197,6 +200,10 @@ class ThemeService : public base::NonThreadSafe,
   // same as the default theme).
   virtual bool UsingNativeTheme() const;
 
+  // Gets the id of the last installed theme. (The theme may have been further
+  // locally customized.)
+  virtual std::string GetThemeID() const;
+
   // This class needs to keep track of the number of theme infobars so that we
   // clean up unused themes.
   void OnInfobarDisplayed();
@@ -205,23 +212,6 @@ class ThemeService : public base::NonThreadSafe,
   // destroyed, uninstalls all themes that aren't the currently selected.
   void OnInfobarDestroyed();
 
-  // Gets the id of the last installed theme for |profile|. (The theme
-  // may have been further locally customized.)
-  static std::string GetThemeIDForProfile(Profile* profile);
-
-  // Save the id of the last theme installed.  If the theme service
-  // already exists, SetTheme() must be used instead.
-  static void SaveThemeIDForProfile(Profile* profile, const std::string& id);
-
-  // Uninstall extensions for themes that are no longer in use.
-  static void RemoveUnusedThemesForProfile(Profile* profile);
-
-  // Gets the id of the last installed theme. (The theme may have been
-  // further locally customized.)
-  //
-  // TODO(akalin): Make everything use GetThemeIDForProfile().
-  virtual std::string GetThemeID() const;
-
   // Convert a bitfield alignment into a string like "top left". Public so that
   // it can be used to generate CSS values. Takes a bitmask of Alignment.
   static std::string AlignmentToString(int alignment);
@@ -250,6 +240,9 @@ class ThemeService : public base::NonThreadSafe,
   // Returns the set of IDR_* resources that should be tinted.
   static const std::set<int>& GetTintableToolbarButtons();
 
+  // Remove preference values for themes that are no longer in use.
+  void RemoveUnusedThemes();
+
   // Returns the syncable service for syncing theme. The returned service is
   // owned by |this| object.
   virtual ThemeSyncableService* GetThemeSyncableService() const;
@@ -281,12 +274,20 @@ class ThemeService : public base::NonThreadSafe,
 
   Profile* profile() { return profile_; }
 
+  // content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
  private:
   friend class ThemeServiceTest;
 
   // Saves the filename of the cached theme pack.
   void SavePackName(const FilePath& pack_path);
 
+  // Save the id of the last theme installed.
+  void SaveThemeID(const std::string& id);
+
   // Implementation of SetTheme() (and the fallback from LoadThemePrefs() in
   // case we don't have a theme pack).
   void BuildFromExtension(const extensions::Extension* extension);
@@ -319,6 +320,8 @@ class ThemeService : public base::NonThreadSafe,
   // The number of infobars currently displayed.
   int number_of_infobars_;
 
+  content::NotificationRegistrar registrar_;
+
   scoped_ptr<ThemeSyncableService> theme_syncable_service_;
 
   DISALLOW_COPY_AND_ASSIGN(ThemeService);
diff --git a/chrome/browser/themes/theme_service_factory.cc b/chrome/browser/themes/theme_service_factory.cc
index 8992f1f..10a3b73 100644
--- a/chrome/browser/themes/theme_service_factory.cc
+++ b/chrome/browser/themes/theme_service_factory.cc
@@ -19,27 +19,22 @@
 // static
 ThemeService* ThemeServiceFactory::GetForProfile(Profile* profile) {
   return static_cast<ThemeService*>(
-      GetInstance()->GetServiceForProfile(profile, true /* create */));
+      GetInstance()->GetServiceForProfile(profile, true));
 }
 
 // static
-ThemeServiceFactory* ThemeServiceFactory::GetInstance() {
-  return Singleton<ThemeServiceFactory>::get();
+const extensions::Extension* ThemeServiceFactory::GetThemeForProfile(
+    Profile* profile) {
+  std::string id = GetForProfile(profile)->GetThemeID();
+  if (id == ThemeService::kDefaultThemeID)
+    return NULL;
+
+  return profile->GetExtensionService()->GetExtensionById(id, false);
 }
 
 // static
-void ThemeServiceFactory::SetThemeForProfile(
-    Profile* profile,
-    const extensions::Extension* theme) {
-  ThemeService* theme_service =
-      static_cast<ThemeService*>(
-          GetInstance()->GetServiceForProfile(profile, false /* create */));
-  if (theme_service) {
-    theme_service->SetTheme(theme);
-  } else {
-    // The theme service will pick up the new theme when it's created.
-    ThemeService::SaveThemeIDForProfile(profile, theme->id());
-  }
+ThemeServiceFactory* ThemeServiceFactory::GetInstance() {
+  return Singleton<ThemeServiceFactory>::get();
 }
 
 ThemeServiceFactory::ThemeServiceFactory()
diff --git a/chrome/browser/themes/theme_service_factory.h b/chrome/browser/themes/theme_service_factory.h
index f74f167..be7b956 100644
--- a/chrome/browser/themes/theme_service_factory.h
+++ b/chrome/browser/themes/theme_service_factory.h
@@ -26,15 +26,10 @@ class ThemeServiceFactory : public ProfileKeyedServiceFactory {
   // still needs a ThemeService to hand back the default theme images.
   static ThemeService* GetForProfile(Profile* profile);
 
-  // If the theme service for |profile| has been instantiated,
-  // immediately sets its theme to |theme|.  Otherwise, saves
-  // |theme|'s ID so that the theme service picks it up when it gets
-  // initialized.
-  //
-  // |theme| must already be installed in the extension service.
-  static void SetThemeForProfile(
-      Profile* profile,
-      const extensions::Extension* theme);
+  // Returns the Extension that implements the theme associated with
+  // |profile|. Returns NULL if the theme is no longer installed, if there is
+  // no installed theme, or the theme was cleared.
+  static const extensions::Extension* GetThemeForProfile(Profile* profile);
 
   static ThemeServiceFactory* GetInstance();
 
diff --git a/chrome/browser/ui/gtk/gtk_theme_service.cc b/chrome/browser/ui/gtk/gtk_theme_service.cc
index 900f204..6dc9012 100644
--- a/chrome/browser/ui/gtk/gtk_theme_service.cc
+++ b/chrome/browser/ui/gtk/gtk_theme_service.cc
@@ -294,6 +294,7 @@ void GtkThemeService::Init(Profile* profile) {
   registrar_.Add(prefs::kUsesSystemTheme,
                  base::Bind(&GtkThemeService::OnUsesSystemThemeChanged,
                             base::Unretained(this)));
+  use_gtk_ = profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
   ThemeService::Init(profile);
 }
 
@@ -335,7 +336,7 @@ bool GtkThemeService::HasCustomImage(int id) const {
   return ThemeService::HasCustomImage(id);
 }
 
-void GtkThemeService::InitThemesFor(content::NotificationObserver* observer) {
+void GtkThemeService::InitThemesFor(NotificationObserver* observer) {
   observer->Observe(chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
                     content::Source<ThemeService>(this),
                     content::NotificationService::NoDetails());
@@ -619,14 +620,6 @@ void GtkThemeService::ClearAllThemeData() {
 }
 
 void GtkThemeService::LoadThemePrefs() {
-  if (ThemeService::UsingDefaultTheme()) {
-    use_gtk_ = profile()->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
-  } else {
-    // Do this here since ThemeServiceFactory::SetThemeForProfile()
-    // doesn't know to set kUsesSystemTheme to false.
-    profile()->GetPrefs()->SetBoolean(prefs::kUsesSystemTheme, false);
-    use_gtk_ = false;
-  }
   if (use_gtk_) {
     LoadGtkValues();
   } else {
diff --git a/chrome/browser/ui/gtk/gtk_theme_service.h b/chrome/browser/ui/gtk/gtk_theme_service.h
index 5368497..b476767 100644
--- a/chrome/browser/ui/gtk/gtk_theme_service.h
+++ b/chrome/browser/ui/gtk/gtk_theme_service.h
@@ -20,10 +20,6 @@
 
 class Profile;
 
-namespace content {
-class NotificationObserver;
-}
-
 namespace extensions {
 class Extension;
 }