[Sync] Simplify themes notifications and themes sync

Change ThemesService to listen to EXTENSION_LOADED instead of
THEME_INSTALLED.  Remove THEME_INSTALLED notification and always send
EXTENSION_INSTALLED.

Remove Extension details from BROWSER_THEME_CHANGED notification.

The two changes above make it so that listeners to BROWSER_THEME_CHANGED
can simply query the ThemeService about the new theme; previously this
was not possible since the theme was only installed and not loaded.

Cut out the now-unneeded code in ThemeChangeProcessor (yay!).

BUG=74831
TEST=

Review URL: http://codereview.chromium.org/6874001

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

8 files changed, 26 insertions, 124 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index d4d7f8d..34427a0 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1657,19 +1657,10 @@ void ExtensionService::OnExtensionInstalled(const Extension* extension) {
     extension_prefs_->SetAllowFileAccess(id, true);
   }
 
-  // If the extension is a theme, tell the profile (and therefore ThemeProvider)
-  // to apply it.
-  if (extension->is_theme()) {
-    NotificationService::current()->Notify(
-        NotificationType::THEME_INSTALLED,
-        Source<Profile>(profile_),
-        Details<const Extension>(extension));
-  } else {
-    NotificationService::current()->Notify(
-        NotificationType::EXTENSION_INSTALLED,
-        Source<Profile>(profile_),
-        Details<const Extension>(extension));
-  }
+  NotificationService::current()->Notify(
+      NotificationType::EXTENSION_INSTALLED,
+      Source<Profile>(profile_),
+      Details<const Extension>(extension));
 
   // Transfer ownership of |extension| to AddExtension.
   AddExtension(scoped_extension);
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 742d6fa..7b22b41 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -474,8 +474,6 @@ class ExtensionServiceTest
                    NotificationService::AllSources());
     registrar_.Add(this, NotificationType::EXTENSION_INSTALLED,
                    NotificationService::AllSources());
-    registrar_.Add(this, NotificationType::THEME_INSTALLED,
-                   NotificationService::AllSources());
   }
 
   virtual void Observe(NotificationType type,
@@ -505,7 +503,6 @@ class ExtensionServiceTest
         break;
       }
       case NotificationType::EXTENSION_INSTALLED:
-      case NotificationType::THEME_INSTALLED:
         installed_ = Details<const Extension>(details).ptr();
         break;
 
diff --git a/chrome/browser/extensions/theme_installed_infobar_delegate.cc b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
index eaf7a7a..e6aa111 100644
--- a/chrome/browser/extensions/theme_installed_infobar_delegate.cc
+++ b/chrome/browser/extensions/theme_installed_infobar_delegate.cc
@@ -100,8 +100,7 @@ void ThemeInstalledInfoBarDelegate::Observe(
   DCHECK_EQ(NotificationType::BROWSER_THEME_CHANGED, type.value);
   // If the new theme is different from what this info bar is associated
   // with, close this info bar since it is no longer relevant.
-  const Extension* extension = Details<const Extension>(details).ptr();
-  if (!extension || theme_id_ != extension->id()) {
+  if (theme_id_ != theme_service_->GetThemeID()) {
     if (tab_contents_ && !tab_contents_->is_being_destroyed()) {
       tab_contents_->RemoveInfoBar(this);
       // The infobar is gone so there is no reason for this delegate to keep
diff --git a/chrome/browser/sync/glue/theme_change_processor.cc b/chrome/browser/sync/glue/theme_change_processor.cc
index 458270d..a75b7eb 100644
--- a/chrome/browser/sync/glue/theme_change_processor.cc
+++ b/chrome/browser/sync/glue/theme_change_processor.cc
@@ -17,15 +17,6 @@
 
 namespace browser_sync {
 
-namespace {
-std::string GetThemeId(const Extension* current_theme) {
-  if (current_theme) {
-    DCHECK(current_theme->is_theme());
-  }
-  return current_theme ? current_theme->id() : "default/system";
-}
-}  // namespace
-
 ThemeChangeProcessor::ThemeChangeProcessor(
     UnrecoverableErrorHandler* error_handler)
     : ChangeProcessor(error_handler),
@@ -40,81 +31,7 @@ void ThemeChangeProcessor::Observe(NotificationType type,
                                    const NotificationDetails& details) {
   DCHECK(running());
   DCHECK(profile_);
-  const Extension* extension = NULL;
-  if (type == NotificationType::EXTENSION_UNLOADED) {
-    extension = Details<UnloadedExtensionInfo>(details)->extension;
-  } else {
-    extension = Details<const Extension>(details).ptr();
-  }
-  ThemeService* theme_provider =
-      ThemeServiceFactory::GetForProfile(profile_);
-  std::string current_or_future_theme_id = theme_provider->GetThemeID();
-  const Extension* current_theme =
-      ThemeServiceFactory::GetThemeForProfile(profile_);
-  switch (type.value) {
-    case NotificationType::BROWSER_THEME_CHANGED:
-      // We pay attention to this notification only when it signifies
-      // that a user changed the theme to the default/system theme, or
-      // when it signifies that a user changed the theme to a custom
-      // one that was already installed.  Otherwise, current_theme
-      // still points to the previous theme until it gets installed
-      // and loaded (and we get an EXTENSION_LOADED notification).
-      VLOG(1) << "Got BROWSER_THEME_CHANGED notification for theme "
-              << GetThemeId(extension);
-      DCHECK_EQ(Source<ThemeService>(source).ptr(),
-                theme_provider);
-      if (extension != NULL) {
-        DCHECK(extension->is_theme());
-        DCHECK_EQ(extension->id(), current_or_future_theme_id);
-        if (!current_theme || (current_theme->id() != extension->id())) {
-          return;
-        }
-      }
-      break;
-    case NotificationType::EXTENSION_LOADED:
-      // We pay attention to this notification only when it signifies
-      // that a theme extension has been loaded because that means
-      // that the user set the current theme to a custom theme that
-      // needed to be downloaded and installed and that it was
-      // installed successfully.
-      DCHECK_EQ(Source<Profile>(source).ptr(), profile_);
-      CHECK(extension);
-      if (!extension->is_theme()) {
-        return;
-      }
-      VLOG(1) << "Got EXTENSION_LOADED notification for theme "
-              << extension->id();
-      DCHECK_EQ(extension->id(), current_or_future_theme_id);
-      DCHECK_EQ(extension, current_theme);
-      break;
-    case NotificationType::EXTENSION_UNLOADED:
-      // We pay attention to this notification only when it signifies
-      // that a theme extension has been unloaded because that means
-      // that the user set the current theme to a custom theme and then
-      // changed his mind and undid it (reverting to the previous
-      // theme).
-      DCHECK_EQ(Source<Profile>(source).ptr(), profile_);
-      CHECK(extension);
-      if (!extension->is_theme()) {
-        return;
-      }
-      VLOG(1) << "Got EXTENSION_UNLOADED notification for theme "
-              << extension->id();
-      extension = current_theme;
-      break;
-    default:
-      LOG(DFATAL) << "Unexpected notification received: " << type.value;
-      break;
-  }
-
-  DCHECK_EQ(extension, current_theme);
-  if (extension) {
-    DCHECK(extension->is_theme());
-  }
-  VLOG(1) << "Theme changed to " << GetThemeId(extension);
-
-  // Here, we know that a theme is being set; the theme is a custom
-  // theme iff extension is non-NULL.
+  DCHECK(type == NotificationType::BROWSER_THEME_CHANGED);
 
   sync_api::WriteTransaction trans(share_handle());
   sync_api::WriteNode node(&trans);
@@ -201,18 +118,11 @@ void ThemeChangeProcessor::StopImpl() {
 
 void ThemeChangeProcessor::StartObserving() {
   DCHECK(profile_);
-  VLOG(1) << "Observing BROWSER_THEME_CHANGED, EXTENSION_LOADED, and "
-             "EXTENSION_UNLOADED";
+  VLOG(1) << "Observing BROWSER_THEME_CHANGED";
   notification_registrar_.Add(
       this, NotificationType::BROWSER_THEME_CHANGED,
       Source<ThemeService>(
           ThemeServiceFactory::GetForProfile(profile_)));
-  notification_registrar_.Add(
-      this, NotificationType::EXTENSION_LOADED,
-      Source<Profile>(profile_));
-  notification_registrar_.Add(
-      this, NotificationType::EXTENSION_UNLOADED,
-      Source<Profile>(profile_));
 }
 
 void ThemeChangeProcessor::StopObserving() {
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
index 72a2dba..5d1c60e 100644
--- a/chrome/browser/themes/theme_service.cc
+++ b/chrome/browser/themes/theme_service.cc
@@ -209,8 +209,12 @@ 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,
-                 NotificationType::THEME_INSTALLED,
+                 NotificationType::EXTENSION_LOADED,
                  Source<Profile>(profile_));
 
   LoadThemePrefs();
@@ -308,7 +312,7 @@ void ThemeService::SetTheme(const Extension* extension) {
   BuildFromExtension(extension);
   SaveThemeID(extension->id());
 
-  NotifyThemeChanged(extension);
+  NotifyThemeChanged();
   UserMetrics::RecordAction(UserMetricsAction("Themes_Installed"), profile_);
 }
 
@@ -333,7 +337,7 @@ void ThemeService::RemoveUnusedThemes() {
 
 void ThemeService::UseDefaultTheme() {
   ClearAllThemeData();
-  NotifyThemeChanged(NULL);
+  NotifyThemeChanged();
   UserMetrics::RecordAction(UserMetricsAction("Themes_Reset"), profile_);
 }
 
@@ -590,13 +594,13 @@ void ThemeService::LoadThemePrefs() {
   }
 }
 
-void ThemeService::NotifyThemeChanged(const Extension* extension) {
+void ThemeService::NotifyThemeChanged() {
   VLOG(1) << "Sending BROWSER_THEME_CHANGED";
   // Redraw!
   NotificationService* service = NotificationService::current();
   service->Notify(NotificationType::BROWSER_THEME_CHANGED,
                   Source<ThemeService>(this),
-                  Details<const Extension>(extension));
+                  NotificationService::NoDetails());
 #if defined(OS_MACOSX)
   NotifyPlatformThemeChanged();
 #endif  // OS_MACOSX
@@ -611,8 +615,11 @@ void ThemeService::FreePlatformCaches() {
 void ThemeService::Observe(NotificationType type,
                            const NotificationSource& source,
                            const NotificationDetails& details) {
-  DCHECK(type == NotificationType::THEME_INSTALLED);
+  DCHECK(type == NotificationType::EXTENSION_LOADED);
   const Extension* extension = Details<const Extension>(details).ptr();
+  if (!extension->is_theme()) {
+    return;
+  }
   SetTheme(extension);
 }
 
diff --git a/chrome/browser/themes/theme_service.h b/chrome/browser/themes/theme_service.h
index af9f589..c4f5df2 100644
--- a/chrome/browser/themes/theme_service.h
+++ b/chrome/browser/themes/theme_service.h
@@ -231,9 +231,7 @@ class ThemeService : public base::NonThreadSafe,
   virtual void LoadThemePrefs();
 
   // Let all the browser views know that themes have changed.
-  // extension is NULL iff the theme is being set to the
-  // default/system theme.
-  virtual void NotifyThemeChanged(const Extension* extension);
+  virtual void NotifyThemeChanged();
 
 #if defined(OS_MACOSX)
   // Let all the browser views know that themes have changed in a platform way.
diff --git a/chrome/browser/ui/gtk/gtk_theme_service.cc b/chrome/browser/ui/gtk/gtk_theme_service.cc
index df6d913..8e433d6 100644
--- a/chrome/browser/ui/gtk/gtk_theme_service.cc
+++ b/chrome/browser/ui/gtk/gtk_theme_service.cc
@@ -348,7 +348,7 @@ void GtkThemeService::SetNativeTheme() {
   profile()->GetPrefs()->SetBoolean(prefs::kUsesSystemTheme, true);
   ClearAllThemeData();
   LoadGtkValues();
-  NotifyThemeChanged(NULL);
+  NotifyThemeChanged();
 }
 
 bool GtkThemeService::UsingDefaultTheme() {
@@ -631,8 +631,8 @@ void GtkThemeService::LoadThemePrefs() {
   RebuildMenuIconSets();
 }
 
-void GtkThemeService::NotifyThemeChanged(const Extension* extension) {
-  ThemeService::NotifyThemeChanged(extension);
+void GtkThemeService::NotifyThemeChanged() {
+  ThemeService::NotifyThemeChanged();
 
   // Notify all GtkChromeButtons of their new rendering mode:
   for (std::vector<GtkWidget*>::iterator it = chrome_buttons_.begin();
@@ -664,7 +664,7 @@ void GtkThemeService::OnStyleSet(GtkWidget* widget,
   if (profile()->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme)) {
     ClearAllThemeData();
     LoadGtkValues();
-    NotifyThemeChanged(NULL);
+    NotifyThemeChanged();
   }
 
   RebuildMenuIconSets();
diff --git a/chrome/browser/ui/gtk/gtk_theme_service.h b/chrome/browser/ui/gtk/gtk_theme_service.h
index df3ca0d..1206546 100644
--- a/chrome/browser/ui/gtk/gtk_theme_service.h
+++ b/chrome/browser/ui/gtk/gtk_theme_service.h
@@ -156,7 +156,7 @@ class GtkThemeService : public ThemeService {
   virtual void LoadThemePrefs();
 
   // Let all the browser views know that themes have changed.
-  virtual void NotifyThemeChanged(const Extension* extension);
+  virtual void NotifyThemeChanged();
 
   // Additionally frees the CairoCachedSurfaces.
   virtual void FreePlatformCaches();