Add a function triggering extension installed to ExtensionRegistryObserver.

This can be replace chrome::NOTIFICATION_EXTENSION_INSTALLED with ExtensionRegistryObserver.

And apply it to WebStoreInstaller and InstallTracker.

R=kalman@chromium.org
BUG=354459
TEST=unit_test, manual(install app from webstore)

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

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

9 files changed, 138 insertions, 63 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 702b7fc..cfb45b9 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1935,6 +1935,9 @@ void ExtensionService::FinishInstallation(const Extension* extension) {
       content::Source<Profile>(profile_),
       content::Details<const extensions::InstalledExtensionInfo>(&details));
 
+  ExtensionRegistry::Get(profile_)
+      ->TriggerOnWillBeInstalled(extension, is_update, old_name);
+
   bool unacknowledged_external = IsUnacknowledgedExternalExtension(extension);
 
   // Unpacked extensions default to allowing file access, but if that has been
diff --git a/chrome/browser/extensions/install_tracker.cc b/chrome/browser/extensions/install_tracker.cc
index 81e72fed..09eec52 100644
--- a/chrome/browser/extensions/install_tracker.cc
+++ b/chrome/browser/extensions/install_tracker.cc
@@ -21,8 +21,6 @@ InstallTracker::InstallTracker(Profile* profile,
     : extension_registry_observer_(this) {
   extension_registry_observer_.Add(ExtensionRegistry::Get(profile));
 
-  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
-      content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
       content::Source<Profile>(profile));
   registrar_.Add(this,
@@ -95,31 +93,10 @@ void InstallTracker::Shutdown() {
   FOR_EACH_OBSERVER(InstallObserver, observers_, OnShutdown());
 }
 
-void InstallTracker::OnExtensionLoaded(content::BrowserContext* browser_context,
-                                       const Extension* extension) {
-  FOR_EACH_OBSERVER(InstallObserver, observers_, OnExtensionLoaded(extension));
-}
-
-void InstallTracker::OnExtensionUnloaded(
-    content::BrowserContext* browser_context,
-    const Extension* extension,
-    UnloadedExtensionInfo::Reason reason) {
-  FOR_EACH_OBSERVER(
-      InstallObserver, observers_, OnExtensionUnloaded(extension));
-}
-
 void InstallTracker::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
   switch (type) {
-    case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
-      const Extension* extension =
-          content::Details<const InstalledExtensionInfo>(details).ptr()->
-              extension;
-      FOR_EACH_OBSERVER(InstallObserver, observers_,
-                        OnExtensionInstalled(extension));
-      break;
-    }
     case chrome::NOTIFICATION_EXTENSION_UNINSTALLED: {
       const Extension* extension =
           content::Details<const Extension>(details).ptr();
@@ -151,6 +128,28 @@ void InstallTracker::Observe(int type,
   }
 }
 
+void InstallTracker::OnExtensionLoaded(content::BrowserContext* browser_context,
+                                       const Extension* extension) {
+  FOR_EACH_OBSERVER(InstallObserver, observers_, OnExtensionLoaded(extension));
+}
+
+void InstallTracker::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UnloadedExtensionInfo::Reason reason) {
+  FOR_EACH_OBSERVER(
+      InstallObserver, observers_, OnExtensionUnloaded(extension));
+}
+
+void InstallTracker::OnExtensionWillBeInstalled(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    bool is_update,
+    const std::string& old_name) {
+  FOR_EACH_OBSERVER(
+      InstallObserver, observers_, OnExtensionInstalled(extension));
+}
+
 void InstallTracker::OnAppsReordered() {
   FOR_EACH_OBSERVER(InstallObserver, observers_, OnAppsReordered());
 }
diff --git a/chrome/browser/extensions/install_tracker.h b/chrome/browser/extensions/install_tracker.h
index 8342ec9..6728cf6 100644
--- a/chrome/browser/extensions/install_tracker.h
+++ b/chrome/browser/extensions/install_tracker.h
@@ -53,7 +53,7 @@ class InstallTracker : public KeyedService,
  private:
   void OnAppsReordered();
 
-  // content::NotificationObserver.
+  // content::NotificationObserver implementation.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
@@ -65,12 +65,16 @@ class InstallTracker : public KeyedService,
       content::BrowserContext* browser_context,
       const Extension* extension,
       UnloadedExtensionInfo::Reason reason) OVERRIDE;
+  virtual void OnExtensionWillBeInstalled(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      bool is_update,
+      const std::string& old_name) OVERRIDE;
 
   ObserverList<InstallObserver> observers_;
   content::NotificationRegistrar registrar_;
   PrefChangeRegistrar pref_change_registrar_;
 
-  // Listen to extension load, unloaded notifications.
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;
 
diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
index 95b961a..512d008 100644
--- a/chrome/browser/extensions/webstore_installer.cc
+++ b/chrome/browser/extensions/webstore_installer.cc
@@ -48,6 +48,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_constants.h"
@@ -277,6 +278,7 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile,
                                      scoped_ptr<Approval> approval,
                                      InstallSource source)
     : content::WebContentsObserver(web_contents),
+      extension_registry_observer_(this),
       profile_(profile),
       delegate_(delegate),
       id_(id),
@@ -290,10 +292,9 @@ WebstoreInstaller::WebstoreInstaller(Profile* profile,
 
   registrar_.Add(this, chrome::NOTIFICATION_CRX_INSTALLER_DONE,
                  content::NotificationService::AllSources());
-  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
-                 content::Source<Profile>(profile->GetOriginalProfile()));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
                  content::Source<CrxInstaller>(NULL));
+  extension_registry_observer_.Add(ExtensionRegistry::Get(profile));
 }
 
 void WebstoreInstaller::Start() {
@@ -375,40 +376,6 @@ void WebstoreInstaller::Observe(int type,
       break;
     }
 
-    case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
-      CHECK(profile_->IsSameProfile(content::Source<Profile>(source).ptr()));
-      const Extension* extension =
-          content::Details<const InstalledExtensionInfo>(details)->extension;
-      if (pending_modules_.empty())
-        return;
-      SharedModuleInfo::ImportInfo info = pending_modules_.front();
-      if (extension->id() != info.extension_id)
-        return;
-      pending_modules_.pop_front();
-
-      if (pending_modules_.empty()) {
-        CHECK_EQ(extension->id(), id_);
-        ReportSuccess();
-      } else {
-        const Version version_required(info.minimum_version);
-        if (version_required.IsValid() &&
-            extension->version()->CompareTo(version_required) < 0) {
-          // It should not happen, CrxInstaller will make sure the version is
-          // equal or newer than version_required.
-          ReportFailure(kDependencyNotFoundError,
-              FAILURE_REASON_DEPENDENCY_NOT_FOUND);
-        } else if (!SharedModuleInfo::IsSharedModule(extension)) {
-          // It should not happen, CrxInstaller will make sure it is a shared
-          // module.
-          ReportFailure(kDependencyNotSharedModuleError,
-              FAILURE_REASON_DEPENDENCY_NOT_SHARED_MODULE);
-        } else {
-          DownloadNextPendingModule();
-        }
-      }
-      break;
-    }
-
     case chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR: {
       CrxInstaller* crx_installer = content::Source<CrxInstaller>(source).ptr();
       CHECK(crx_installer);
@@ -432,6 +399,41 @@ void WebstoreInstaller::Observe(int type,
   }
 }
 
+void WebstoreInstaller::OnExtensionWillBeInstalled(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    bool is_update,
+    const std::string& old_name) {
+  CHECK(profile_->IsSameProfile(Profile::FromBrowserContext(browser_context)));
+  if (pending_modules_.empty())
+    return;
+  SharedModuleInfo::ImportInfo info = pending_modules_.front();
+  if (extension->id() != info.extension_id)
+    return;
+  pending_modules_.pop_front();
+
+  if (pending_modules_.empty()) {
+    CHECK_EQ(extension->id(), id_);
+    ReportSuccess();
+  } else {
+    const Version version_required(info.minimum_version);
+    if (version_required.IsValid() &&
+        extension->version()->CompareTo(version_required) < 0) {
+      // It should not happen, CrxInstaller will make sure the version is
+      // equal or newer than version_required.
+      ReportFailure(kDependencyNotFoundError,
+                    FAILURE_REASON_DEPENDENCY_NOT_FOUND);
+    } else if (!SharedModuleInfo::IsSharedModule(extension)) {
+      // It should not happen, CrxInstaller will make sure it is a shared
+      // module.
+      ReportFailure(kDependencyNotSharedModuleError,
+                    FAILURE_REASON_DEPENDENCY_NOT_SHARED_MODULE);
+    } else {
+      DownloadNextPendingModule();
+    }
+  }
+}
+
 void WebstoreInstaller::InvalidateDelegate() {
   delegate_ = NULL;
 }
diff --git a/chrome/browser/extensions/webstore_installer.h b/chrome/browser/extensions/webstore_installer.h
index c0e2d50..c2d1646 100644
--- a/chrome/browser/extensions/webstore_installer.h
+++ b/chrome/browser/extensions/webstore_installer.h
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/scoped_observer.h"
 #include "base/supports_user_data.h"
 #include "base/timer/timer.h"
 #include "base/values.h"
@@ -22,6 +23,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "extensions/browser/extension_registry_observer.h"
 #include "extensions/common/manifest_handlers/shared_module_info.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
@@ -40,14 +42,17 @@ namespace extensions {
 
 class CrxInstaller;
 class Extension;
+class ExtensionRegistry;
 class Manifest;
 
 // Downloads and installs extensions from the web store.
 class WebstoreInstaller : public content::NotificationObserver,
+                          public ExtensionRegistryObserver,
                           public content::DownloadItem::Observer,
                           public content::WebContentsObserver,
                           public base::RefCountedThreadSafe<
-  WebstoreInstaller, content::BrowserThread::DeleteOnUIThread> {
+                              WebstoreInstaller,
+                              content::BrowserThread::DeleteOnUIThread> {
  public:
   enum InstallSource {
     // Inline installs trigger slightly different behavior (install source
@@ -188,11 +193,18 @@ class WebstoreInstaller : public content::NotificationObserver,
   // Starts downloading and installing the extension.
   void Start();
 
-  // content::NotificationObserver
+  // content::NotificationObserver.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // ExtensionRegistryObserver.
+  virtual void OnExtensionWillBeInstalled(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      bool is_update,
+      const std::string& old_name) OVERRIDE;
+
   // Removes the reference to the delegate passed in the constructor. Used when
   // the delegate object must be deleted before this object.
   void InvalidateDelegate();
@@ -249,6 +261,8 @@ class WebstoreInstaller : public content::NotificationObserver,
   void RecordInterrupt(const content::DownloadItem* download) const;
 
   content::NotificationRegistrar registrar_;
+  ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
+      extension_registry_observer_;
   Profile* profile_;
   Delegate* delegate_;
   std::string id_;
diff --git a/extensions/browser/extension_registry.cc b/extensions/browser/extension_registry.cc
index ac6c274..76783f0 100644
--- a/extensions/browser/extension_registry.cc
+++ b/extensions/browser/extension_registry.cc
@@ -53,6 +53,18 @@ void ExtensionRegistry::TriggerOnUnloaded(
                     OnExtensionUnloaded(browser_context_, extension, reason));
 }
 
+void ExtensionRegistry::TriggerOnWillBeInstalled(const Extension* extension,
+                                                 bool is_update,
+                                                 const std::string& old_name) {
+  DCHECK(is_update ==
+         GenerateInstalledExtensionsSet()->Contains(extension->id()));
+  DCHECK(is_update == !old_name.empty());
+  FOR_EACH_OBSERVER(ExtensionRegistryObserver,
+                    observers_,
+                    OnExtensionWillBeInstalled(
+                        browser_context_, extension, is_update, old_name));
+}
+
 const Extension* ExtensionRegistry::GetExtensionById(const std::string& id,
                                                      int include_mask) const {
   std::string lowercase_id = StringToLowerASCII(id);
diff --git a/extensions/browser/extension_registry.h b/extensions/browser/extension_registry.h
index d8df24b..65ee751 100644
--- a/extensions/browser/extension_registry.h
+++ b/extensions/browser/extension_registry.h
@@ -74,6 +74,14 @@ class ExtensionRegistry : public KeyedService {
   void TriggerOnUnloaded(const Extension* extension,
                          UnloadedExtensionInfo::Reason reason);
 
+  // If this is a fresh install then |is_update| is false and there must not be
+  // any installed extension with |extension|'s ID. If this is an update then
+  // |is_update| is true and must be an installed extension with |extension|'s
+  // ID, and |old_name| must be non-empty.
+  void TriggerOnWillBeInstalled(const Extension* extension,
+                                bool is_update,
+                                const std::string& old_name);
+
   // Find an extension by ID using |include_mask| to pick the sets to search:
   //  * enabled_extensions()     --> ExtensionRegistry::ENABLED
   //  * disabled_extensions()    --> ExtensionRegistry::DISABLED
diff --git a/extensions/browser/extension_registry_observer.h b/extensions/browser/extension_registry_observer.h
index 5880add..6ad0666 100644
--- a/extensions/browser/extension_registry_observer.h
+++ b/extensions/browser/extension_registry_observer.h
@@ -33,6 +33,21 @@ class ExtensionRegistryObserver {
   virtual void OnExtensionUnloaded(content::BrowserContext* browser_context,
                                    const Extension* extension,
                                    UnloadedExtensionInfo::Reason reason) {}
+
+  // Called when |extension| is about to be installed. |is_update| is true if
+  // the installation is the result of it updating, in which case |old_name| is
+  // the name of the extension's previous version.
+  // The ExtensionRegistry will not be tracking |extension| at the time this
+  // event is fired, but will be immediately afterwards (note: not necessarily
+  // enabled; it might be installed in the disabled or even blacklisted sets,
+  // for example).
+  // Note that it's much more common to care about extensions being loaded
+  // (OnExtensionLoaded).
+  virtual void OnExtensionWillBeInstalled(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      bool is_update,
+      const std::string& old_name) {}
 };
 
 }  // namespace extensions
diff --git a/extensions/browser/extension_registry_unittest.cc b/extensions/browser/extension_registry_unittest.cc
index f212f6c..8281ee0 100644
--- a/extensions/browser/extension_registry_unittest.cc
+++ b/extensions/browser/extension_registry_unittest.cc
@@ -7,6 +7,7 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
+#include "base/strings/string_util.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/test_util.h"
@@ -37,10 +38,12 @@ class TestObserver : public ExtensionRegistryObserver {
   void Reset() {
     loaded_.clear();
     unloaded_.clear();
+    installed_.clear();
   }
 
   const ExtensionList& loaded() { return loaded_; }
   const ExtensionList& unloaded() { return unloaded_; }
+  const ExtensionList& installed() { return installed_; }
 
  private:
   virtual void OnExtensionLoaded(content::BrowserContext* browser_context,
@@ -55,8 +58,17 @@ class TestObserver : public ExtensionRegistryObserver {
     unloaded_.push_back(extension);
   }
 
+  virtual void OnExtensionWillBeInstalled(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      bool is_update,
+      const std::string& old_name) OVERRIDE {
+    installed_.push_back(extension);
+  }
+
   ExtensionList loaded_;
   ExtensionList unloaded_;
+  ExtensionList installed_;
 };
 
 TEST_F(ExtensionRegistryTest, FillAndClearRegistry) {
@@ -215,13 +227,19 @@ TEST_F(ExtensionRegistryTest, Observer) {
 
   EXPECT_TRUE(observer.loaded().empty());
   EXPECT_TRUE(observer.unloaded().empty());
+  EXPECT_TRUE(observer.installed().empty());
 
   scoped_refptr<const Extension> extension =
       test_util::CreateExtensionWithID("id");
 
+  registry.TriggerOnWillBeInstalled(extension, false, base::EmptyString());
+  EXPECT_TRUE(HasSingleExtension(observer.installed(), extension.get()));
+
   registry.AddEnabled(extension);
   registry.TriggerOnLoaded(extension);
 
+  registry.TriggerOnWillBeInstalled(extension, true, "foo");
+
   EXPECT_TRUE(HasSingleExtension(observer.loaded(), extension.get()));
   EXPECT_TRUE(observer.unloaded().empty());
   observer.Reset();