Changed EXTENSION_UNINSTALLED notification to happen after uninstallation.

The important part is that it comes after the EXTENSION_UNLOADED
notification is sent.  This makes it easier on the listeners, as they
can assume that extension notifications other than EXTENSION_UNINSTALLED
are sent for currently-installed extensions.

BUG=54415
TEST=BackgroundModeManagerTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60834

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60848

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

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

8 files changed, 53 insertions, 34 deletions

diff --git a/chrome/browser/background_mode_manager.cc b/chrome/browser/background_mode_manager.cc
index ba952cb..8946e3c 100644
--- a/chrome/browser/background_mode_manager.cc
+++ b/chrome/browser/background_mode_manager.cc
@@ -188,6 +188,20 @@ void BackgroundModeManager::SetLaunchOnStartupResetAllowed(bool allowed) {
                                    allowed);
 }
 
+namespace {
+
+bool HasBackgroundAppPermission(
+    const std::set<std::string>& api_permissions) {
+  return Extension::HasApiPermission(
+      api_permissions, Extension::kBackgroundPermission);
+}
+
+bool IsBackgroundApp(const Extension& extension) {
+  return HasBackgroundAppPermission(extension.api_permissions());
+}
+
+}  // namespace
+
 void BackgroundModeManager::Observe(NotificationType type,
                                     const NotificationSource& source,
                                     const NotificationDetails& details) {
@@ -202,19 +216,21 @@ void BackgroundModeManager::Observe(NotificationType type,
 #endif
       break;
     case NotificationType::EXTENSION_LOADED:
-      if (IsBackgroundApp(Details<Extension>(details).ptr()))
+      if (IsBackgroundApp(*Details<Extension>(details).ptr()))
         OnBackgroundAppLoaded();
       break;
     case NotificationType::EXTENSION_UNLOADED:
-      if (IsBackgroundApp(Details<Extension>(details).ptr()))
+      if (IsBackgroundApp(*Details<Extension>(details).ptr()))
         OnBackgroundAppUnloaded();
       break;
     case NotificationType::EXTENSION_INSTALLED:
-      if (IsBackgroundApp(Details<Extension>(details).ptr()))
+      if (IsBackgroundApp(*Details<Extension>(details).ptr()))
         OnBackgroundAppInstalled();
       break;
     case NotificationType::EXTENSION_UNINSTALLED:
-      if (IsBackgroundApp(Details<Extension>(details).ptr()))
+      if (HasBackgroundAppPermission(
+              Details<UninstalledExtensionInfo>(details).ptr()->
+              extension_api_permissions))
         OnBackgroundAppUninstalled();
       break;
     case NotificationType::APP_TERMINATING:
@@ -236,11 +252,6 @@ void BackgroundModeManager::Observe(NotificationType type,
   }
 }
 
-bool BackgroundModeManager::IsBackgroundApp(Extension* extension) {
-  return extension->HasApiPermission(Extension::kBackgroundPermission);
-}
-
-
 void BackgroundModeManager::OnBackgroundModePrefChanged() {
   // Background mode has been enabled/disabled in preferences, so update our
   // state accordingly.
@@ -308,9 +319,9 @@ void BackgroundModeManager::OnBackgroundAppInstalled() {
 }
 
 void BackgroundModeManager::OnBackgroundAppUninstalled() {
-  // When uninstalling a background app, disable launch on startup if it's the
-  // last one.
-  if (IsBackgroundModeEnabled() && background_app_count_ == 1)
+  // When uninstalling a background app, disable launch on startup if
+  // we have no more background apps.
+  if (IsBackgroundModeEnabled() && background_app_count_ == 0)
     EnableLaunchOnStartup(false);
 }
 
diff --git a/chrome/browser/background_mode_manager.h b/chrome/browser/background_mode_manager.h
index 52f37da..9c3de16 100644
--- a/chrome/browser/background_mode_manager.h
+++ b/chrome/browser/background_mode_manager.h
@@ -86,9 +86,6 @@ class BackgroundModeManager
   // Invoked when the kBackgroundModeEnabled preference has changed.
   void OnBackgroundModePrefChanged();
 
-  // Returns true if the passed extension is a background app.
-  bool IsBackgroundApp(Extension* extension);
-
   // Returns true if the background mode preference is enabled
   bool IsBackgroundModeEnabled();
 
diff --git a/chrome/browser/background_mode_manager_unittest.cc b/chrome/browser/background_mode_manager_unittest.cc
index 075db814..4c43610 100644
--- a/chrome/browser/background_mode_manager_unittest.cc
+++ b/chrome/browser/background_mode_manager_unittest.cc
@@ -56,15 +56,15 @@ TEST_F(BackgroundModeManagerTest, BackgroundAppInstallUninstall) {
   TestingProfile profile;
   TestBackgroundModeManager manager(&profile, command_line_.get());
   // Call to AppInstalled() will cause chrome to be set to launch on startup,
-  // and call to AppUninstalling() set chrome to not launch on startup.
+  // and call to AppUninstalled() set chrome to not launch on startup.
   EXPECT_CALL(manager, EnableLaunchOnStartup(true));
   EXPECT_CALL(manager, CreateStatusTrayIcon());
-  EXPECT_CALL(manager, EnableLaunchOnStartup(false));
   EXPECT_CALL(manager, RemoveStatusTrayIcon());
+  EXPECT_CALL(manager, EnableLaunchOnStartup(false));
   manager.OnBackgroundAppInstalled();
   manager.OnBackgroundAppLoaded();
-  manager.OnBackgroundAppUninstalled();
   manager.OnBackgroundAppUnloaded();
+  manager.OnBackgroundAppUninstalled();
 }
 
 TEST_F(BackgroundModeManagerTest, BackgroundPrefDisabled) {
@@ -72,15 +72,15 @@ TEST_F(BackgroundModeManagerTest, BackgroundPrefDisabled) {
   TestingProfile profile;
   profile.GetPrefs()->SetBoolean(prefs::kBackgroundModeEnabled, false);
   TestBackgroundModeManager manager(&profile, command_line_.get());
+  EXPECT_CALL(manager, CreateStatusTrayIcon()).Times(0);
   // Should not change launch on startup status when installing/uninstalling
   // if background mode is disabled.
   EXPECT_CALL(manager, EnableLaunchOnStartup(true)).Times(0);
-  EXPECT_CALL(manager, CreateStatusTrayIcon()).Times(0);
   manager.OnBackgroundAppInstalled();
   manager.OnBackgroundAppLoaded();
   EXPECT_FALSE(BrowserList::WillKeepAlive());
-  manager.OnBackgroundAppUninstalled();
   manager.OnBackgroundAppUnloaded();
+  manager.OnBackgroundAppUninstalled();
 }
 
 TEST_F(BackgroundModeManagerTest, BackgroundPrefDynamicDisable) {
diff --git a/chrome/browser/extensions/extension_management_api.cc b/chrome/browser/extensions/extension_management_api.cc
index 738c6cf..fe6888a 100644
--- a/chrome/browser/extensions/extension_management_api.cc
+++ b/chrome/browser/extensions/extension_management_api.cc
@@ -215,9 +215,8 @@ void ExtensionManagementEventRouter::Observe(
 
   ListValue args;
   if (event_name == events::kOnExtensionUninstalled) {
-    // TODO(akalin) - change this to get the id from UninstalledExtensionInfo
-    // when re-landing change to how we send the uninstall notification.
-    std::string extension_id = Details<Extension>(details).ptr()->id();
+    const std::string& extension_id =
+        Details<UninstalledExtensionInfo>(details).ptr()->extension_id;
     args.Append(Value::CreateStringValue(extension_id));
   } else {
     Extension* extension = Details<Extension>(details).ptr();
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 737f689..03699fa 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -768,16 +768,11 @@ void ExtensionsService::UninstallExtension(const std::string& extension_id,
   // Callers should not send us nonexistent extensions.
   DCHECK(extension);
 
-  // Notify interested parties that we're uninstalling this extension.
-  NotificationService::current()->Notify(
-      NotificationType::EXTENSION_UNINSTALLED,
-      Source<Profile>(profile_),
-      Details<Extension>(extension));
-
   // Get hold of information we need after unloading, since the extension
   // pointer will be invalid then.
   GURL extension_url(extension->url());
   Extension::Location location(extension->location());
+  UninstalledExtensionInfo uninstalled_extension_info(*extension);
 
   // Also copy the extension identifier since the reference might have been
   // obtained via Extension::id().
@@ -804,6 +799,12 @@ void ExtensionsService::UninstallExtension(const std::string& extension_id,
   }
 
   ClearExtensionData(extension_url);
+
+  // Notify interested parties that we've uninstalled this extension.
+  NotificationService::current()->Notify(
+      NotificationType::EXTENSION_UNINSTALLED,
+      Source<Profile>(profile_),
+      Details<UninstalledExtensionInfo>(&uninstalled_extension_info));
 }
 
 void ExtensionsService::ClearExtensionData(const GURL& extension_url) {
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 30d0e17..fbfc73d 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -2037,3 +2037,8 @@ ExtensionInfo::ExtensionInfo(const DictionaryValue* manifest,
 
 ExtensionInfo::~ExtensionInfo() {
 }
+
+UninstalledExtensionInfo::UninstalledExtensionInfo(
+    const Extension& extension)
+    : extension_id(extension.id()),
+      extension_api_permissions(extension.api_permissions()) {}
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index acb8edf..a28259c 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -626,4 +626,13 @@ struct ExtensionInfo {
   DISALLOW_COPY_AND_ASSIGN(ExtensionInfo);
 };
 
+// Struct used for the details of the EXTENSION_UNINSTALLED
+// notification.
+struct UninstalledExtensionInfo {
+  explicit UninstalledExtensionInfo(const Extension& extension);
+
+  std::string extension_id;
+  std::set<std::string> extension_api_permissions;
+};
+
 #endif  // CHROME_COMMON_EXTENSIONS_EXTENSION_H_
diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h
index 541aaa9..d903ffb 100644
--- a/chrome/common/notification_type.h
+++ b/chrome/common/notification_type.h
@@ -797,11 +797,8 @@ class NotificationType {
     // details about why the install failed.
     EXTENSION_INSTALL_ERROR,
 
-    // Sent when a new extension is being uninstalled. When this notification
-    // is sent, the ExtensionsService still is tracking this extension (it has
-    // not been unloaded yet). This will be followed by an EXTENSION_UNLOADED
-    // or EXTENSION_UNLOADED_DISABLED when the extension is actually unloaded.
-    // The details are an Extension and the source is a Profile.
+    // Sent when an extension has been uninstalled.  The details are
+    // an UninstalledExtensionInfo struct and the source is a Profile.
     EXTENSION_UNINSTALLED,
 
     // Sent when an extension is unloaded. This happens when an extension is