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.
[email protected]
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
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 702b7fc4..cfb45b9 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1935,6 +1935,9 @@
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 @@
: 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 @@
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::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 @@
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 @@
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 @@
scoped_ptr<Approval> approval,
InstallSource source)
: content::WebContentsObserver(web_contents),
+ extension_registry_observer_(this),
profile_(profile),
delegate_(delegate),
id_(id),
@@ -290,10 +292,9 @@
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 @@
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::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..c2d16468 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 @@
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 @@
// 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 @@
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 @@
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 d8df24b36..65ee7517 100644
--- a/extensions/browser/extension_registry.h
+++ b/extensions/browser/extension_registry.h
@@ -74,6 +74,14 @@
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..6ad06661 100644
--- a/extensions/browser/extension_registry_observer.h
+++ b/extensions/browser/extension_registry_observer.h
@@ -33,6 +33,21 @@
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 f212f6c8..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 @@
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 @@
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 @@
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();
