Refactor code to make it more testable.
There's an extant problem with external extension providers reporting
that they're ready without actually setting their ready state. This change
allows the implementer of OnExternalProviderReady() to assert that the
caller's ready state is true.
TEST=added
Review URL: http://codereview.chromium.org/8344073
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@106627 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 7c71eb17..e23bba9 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -443,7 +443,7 @@
}
pending_extension_manager()->AddFromExternalUpdateUrl(
id, update_url, location);
- external_extension_url_added_ |= true;
+ external_extension_url_added_ = true;
}
// If a download url matches one of these patterns and has a referrer of the
@@ -2111,17 +2111,21 @@
provider->VisitRegisteredExtension();
}
- // Uninstall of unclaimed extensions will happen after all the providers
- // had reported ready. Every provider calls OnExternalProviderReady()
- // when it finishes, and OnExternalProviderReady() only acts when all
- // providers are ready. In case there are no providers, we call it
- // to trigger removal of extensions that used to have an external source.
- if (external_extension_providers_.empty())
- OnExternalProviderReady();
+ // Do any required work that we would have done after completion of all
+ // providers.
+ if (external_extension_providers_.empty()) {
+ OnAllExternalProvidersReady();
+ }
}
-void ExtensionService::OnExternalProviderReady() {
+void ExtensionService::OnExternalProviderReady(
+ const ExternalExtensionProviderInterface* provider) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+#if 0
+ // TODO(rogerta): Remove guard and this comment when bug described
+ // in http://codereview.chromium.org/8245018/ is fixed.
+ CHECK(provider->IsReady());
+#endif
// An external provider has finished loading. We only take action
// if all of them are finished. So we check them first.
@@ -2133,7 +2137,13 @@
return;
}
- // All the providers are ready. Install any pending extensions.
+ OnAllExternalProvidersReady();
+}
+
+void ExtensionService::OnAllExternalProvidersReady() {
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ // Install any pending extensions.
if (external_extension_url_added_ && updater()) {
external_extension_url_added_ = false;
updater()->CheckNow();
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index cd16db93..aa711211 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -514,7 +514,10 @@
Extension::Location location)
OVERRIDE;
- virtual void OnExternalProviderReady() OVERRIDE;
+ virtual void OnExternalProviderReady(
+ const ExternalExtensionProviderInterface* provider) OVERRIDE;
+
+ void OnAllExternalProvidersReady();
// Once all external providers are done, generates any needed alerts about
// extensions.
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index b23dc47..c7a2830 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -149,7 +149,7 @@
visitor_->OnExternalExtensionFileFound(
i->first, version.get(), i->second.second, location_);
}
- visitor_->OnExternalProviderReady();
+ visitor_->OnExternalProviderReady(this);
}
virtual bool HasExtension(const std::string& id) const {
@@ -172,7 +172,7 @@
return true;
}
- virtual bool IsReady() {
+ virtual bool IsReady() const {
return true;
}
@@ -306,8 +306,10 @@
}
}
- virtual void OnExternalProviderReady() {
- EXPECT_TRUE(provider_->IsReady());
+ virtual void OnExternalProviderReady(
+ const ExternalExtensionProviderInterface* provider) {
+ EXPECT_EQ(provider, provider_.get());
+ EXPECT_TRUE(provider->IsReady());
}
private:
@@ -1269,7 +1271,8 @@
// If we don't check whether the extension is loaded before we uninstall it
// in CheckExternalUninstall, a crash will happen here because we will get or
// dereference a NULL pointer (extension) inside UninstallExtension.
- service_->OnExternalProviderReady();
+ MockExtensionProvider provider(NULL, Extension::EXTERNAL_REGISTRY);
+ service_->OnExternalProviderReady(&provider);
}
// Test that external extensions with incorrect IDs are not installed.
@@ -3098,7 +3101,7 @@
provider->RemoveExtension(good_crx);
loaded_.clear();
- service_->OnExternalProviderReady();
+ service_->OnExternalProviderReady(provider);
loop_.RunAllPending();
ASSERT_EQ(0u, loaded_.size());
ValidatePrefKeyCount(0);
diff --git a/chrome/browser/extensions/external_extension_provider_impl.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
index 5b7c308..60769d6 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -73,7 +73,7 @@
if (profile_) {
ExtensionService* extension_service = profile_->GetExtensionService();
if (extension_service && extension_service->HasApps()) {
- service()->OnExternalProviderReady();
+ service()->OnExternalProviderReady(this);
return;
}
}
@@ -260,14 +260,14 @@
prefs_->Remove(*it, NULL);
}
- service_->OnExternalProviderReady();
+ service_->OnExternalProviderReady(this);
}
void ExternalExtensionProviderImpl::ServiceShutdown() {
service_ = NULL;
}
-bool ExternalExtensionProviderImpl::IsReady() {
+bool ExternalExtensionProviderImpl::IsReady() const {
return ready_;
}
diff --git a/chrome/browser/extensions/external_extension_provider_impl.h b/chrome/browser/extensions/external_extension_provider_impl.h
index d717d41..cf4c7e5d 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.h
+++ b/chrome/browser/extensions/external_extension_provider_impl.h
@@ -60,7 +60,7 @@
Extension::Location* location,
scoped_ptr<Version>* version) const OVERRIDE;
- virtual bool IsReady();
+ virtual bool IsReady() const;
static const char kLocation[];
static const char kState[];
diff --git a/chrome/browser/extensions/external_extension_provider_interface.h b/chrome/browser/extensions/external_extension_provider_interface.h
index 7ad44c3d..f01408ee7 100644
--- a/chrome/browser/extensions/external_extension_provider_interface.h
+++ b/chrome/browser/extensions/external_extension_provider_interface.h
@@ -36,9 +36,13 @@
const GURL& update_url,
Extension::Location location) = 0;
- // Called after all the external extensions have been reported through
- // the above two methods.
- virtual void OnExternalProviderReady() = 0;
+ // Called after all the external extensions have been reported
+ // through the above two methods. |provider| is a pointer to the
+ // provider that is now ready (typically this), and the
+ // implementation of OnExternalProviderReady() should be able to
+ // safely assert that provider->IsReady().
+ virtual void OnExternalProviderReady(
+ const ExternalExtensionProviderInterface* provider) = 0;
protected:
virtual ~VisitorInterface() {}
@@ -67,7 +71,7 @@
// Determines if this provider had loaded the list of external extensions
// from its source.
- virtual bool IsReady() = 0;
+ virtual bool IsReady() const = 0;
};
typedef std::vector<linked_ptr<ExternalExtensionProviderInterface> >
diff --git a/chrome/browser/extensions/external_policy_extension_loader_unittest.cc b/chrome/browser/extensions/external_policy_extension_loader_unittest.cc
index 5ac03b1..72d4991 100644
--- a/chrome/browser/extensions/external_policy_extension_loader_unittest.cc
+++ b/chrome/browser/extensions/external_policy_extension_loader_unittest.cc
@@ -87,8 +87,10 @@
EXPECT_NE(false, remaining_extensions->Remove(ext_str, NULL));
}
- virtual void OnExternalProviderReady() {
- EXPECT_TRUE(provider_->IsReady());
+ virtual void OnExternalProviderReady(
+ const ExternalExtensionProviderInterface* provider) {
+ EXPECT_EQ(provider, provider_.get());
+ EXPECT_TRUE(provider->IsReady());
}
private:
