Clean up threading model of external extension providers

chrome/browser/extensions/external_extension_provider_impl.cc

Index: chrome/browser/extensions/external_extension_provider_impl.cc
diff --git a/chrome/browser/extensions/stateful_external_extension_provider.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
similarity index 61%
rename from chrome/browser/extensions/stateful_external_extension_provider.cc
rename to chrome/browser/extensions/external_extension_provider_impl.cc
index fd1a42af1eb3328390ada2b6d3d3ac39b7254955..84c7f3a9290ea5fa0f748fb254010bd7f7d60d63 100644
--- a/chrome/browser/extensions/stateful_external_extension_provider.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -2,43 +2,64 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/extensions/stateful_external_extension_provider.h"
+#include "chrome/browser/extensions/external_extension_provider_impl.h"
 
 #include "app/app_paths.h"
 #include "base/file_path.h"
 #include "base/logging.h"
+#include "base/linked_ptr.h"
 #include "base/path_service.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_thread.h"
-
-namespace {
-
-// Constants for keeping track of extension preferences.
-const char kLocation[] = "location";
-const char kState[] = "state";
-const char kExternalCrx[] = "external_crx";
-const char kExternalVersion[] = "external_version";
-const char kExternalUpdateUrl[] = "external_update_url";
-
-}
-
-StatefulExternalExtensionProvider::StatefulExternalExtensionProvider(
+#include "chrome/browser/extensions/external_extension_provider.h"
+#include "chrome/browser/extensions/external_policy_extension_loader.h"
+#include "chrome/browser/extensions/external_pref_extension_loader.h"
+#include "chrome/browser/profiles/profile.h"
+
+#if defined(OS_WIN)
+#include "chrome/browser/extensions/external_registry_extension_loader_win.h"
+#endif
+
+// Constants for keeping track of extension preferences in a dictionary.
+const char ExternalExtensionProviderImpl::kLocation[] = "location";
+const char ExternalExtensionProviderImpl::kState[] = "state";
+const char ExternalExtensionProviderImpl::kExternalCrx[] = "external_crx";
+const char ExternalExtensionProviderImpl::kExternalVersion[] =
+    "external_version";
+const char ExternalExtensionProviderImpl::kExternalUpdateUrl[] =
+    "external_update_url";
+
+ExternalExtensionProviderImpl::ExternalExtensionProviderImpl(
+    Visitor* service,
+    ExternalExtensionLoader* loader,
     Extension::Location crx_location,
     Extension::Location download_location)
   : crx_location_(crx_location),
-    download_location_(download_location) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    download_location_(download_location),
+    service_(service),
+    prefs_(NULL),
+    ready_(false),
+    loader_(loader) {
+  loader_->Init(this);
 }
 
-StatefulExternalExtensionProvider::~StatefulExternalExtensionProvider() {
+ExternalExtensionProviderImpl::~ExternalExtensionProviderImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  loader_->OwnerShutdown();
 }
 
-void StatefulExternalExtensionProvider::VisitRegisteredExtension(
-    Visitor* visitor) const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(prefs_.get());
+void ExternalExtensionProviderImpl::VisitRegisteredExtension() const {
+  loader_->StartLoading();
+}
+
+void ExternalExtensionProviderImpl::SetPrefs(DictionaryValue* prefs) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  prefs_.reset(prefs);
+  if (!service_) return;

[Sam Kerner (Chrome), 2011/01/04 14:06:00]

Can this happen?  I see two ways:  User called ServiceShutdown(), or passed
service=NULL in the constructor.  Both seem like the sort of misuse that should
trigger a CHECK.  Is there another way I missed?

[gfeher, 2011/01/04 23:37:09]

It is possible that ExtensionService calls ServiceShutdown while |loader_| is
working on the FILE thread. The loader will call |SetPrefs| after it has
finished with it's stuff.

+
+  ready_ = true;
   for (DictionaryValue::key_iterator i = prefs_->begin_keys();
        i != prefs_->end_keys(); ++i) {
     const std::string& extension_id = *i;
@@ -100,7 +121,7 @@ void StatefulExternalExtensionProvider::VisitRegisteredExtension(
                      << external_version << "\".";
         continue;
       }
-      visitor->OnExternalExtensionFileFound(extension_id, version.get(), path,
+      service_->OnExternalExtensionFileFound(extension_id, version.get(), path,
                                             crx_location_);
     } else { // if (has_external_update_url)
       DCHECK(has_external_update_url);  // Checking of keys above ensures this.
@@ -117,24 +138,36 @@ void StatefulExternalExtensionProvider::VisitRegisteredExtension(
                      << "\".";
         continue;
       }
-      visitor->OnExternalExtensionUpdateUrlFound(
+      service_->OnExternalExtensionUpdateUrlFound(
           extension_id, update_url, download_location_);
     }
   }
+  ready_ = true;
+  service_->OnExternalProviderReady();
+}
+
+void ExternalExtensionProviderImpl::ServiceShutdown() {
+  service_ = NULL;
 }
 
-bool StatefulExternalExtensionProvider::HasExtension(
+bool ExternalExtensionProviderImpl::IsReady() {
+  return ready_;
+}
+
+bool ExternalExtensionProviderImpl::HasExtension(
     const std::string& id) const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_.get());
+  DCHECK(ready_);
   return prefs_->HasKey(id);
 }
 
-bool StatefulExternalExtensionProvider::GetExtensionDetails(
+bool ExternalExtensionProviderImpl::GetExtensionDetails(
     const std::string& id, Extension::Location* location,
     scoped_ptr<Version>* version) const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_.get());
+  DCHECK(ready_);
   DictionaryValue* extension = NULL;
   if (!prefs_->GetDictionary(id, &extension))
     return false;
@@ -164,6 +197,32 @@ bool StatefulExternalExtensionProvider::GetExtensionDetails(
   return true;
 }
 
-void StatefulExternalExtensionProvider::set_prefs(DictionaryValue* prefs) {
-  prefs_.reset(prefs);
+// static
+void ExternalExtensionProviderImpl::CreateExternalProviders(
+    Visitor* service,
+    Profile* profile,
+    ProviderCollection* provider_list) {
+  provider_list->push_back(
+      linked_ptr<ExternalExtensionProvider>(
+          new ExternalExtensionProviderImpl(
+              service,
+              new ExternalPrefExtensionLoader,
+              Extension::EXTERNAL_PREF,
+              Extension::EXTERNAL_PREF_DOWNLOAD)));
+#if defined(OS_WIN)
+  provider_list->push_back(
+      linked_ptr<ExternalExtensionProvider>(
+          new ExternalExtensionProviderImpl(
+              service,
+              new ExternalRegistryExtensionLoader,
+              Extension::EXTERNAL_REGISTRY,
+              Extension::INVALID)));
+#endif
+  provider_list->push_back(
+      linked_ptr<ExternalExtensionProvider>(
+          new ExternalExtensionProviderImpl(
+              service,
+              new ExternalPolicyExtensionLoader(profile),
+              Extension::INVALID,
+              Extension::EXTERNAL_POLICY_DOWNLOAD)));
 }