Clean up threading model of external extension providers

chrome/browser/extensions/extension_service_unittest.cc

Index: chrome/browser/extensions/extension_service_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index d9f8596c71da161c633d0460e81425e25400d134..15fd66aa9dd54949c3a72c4ef7a347fe087416e6 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -28,8 +28,9 @@
 #include "chrome/browser/extensions/extension_creator.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/extensions/external_extension_provider.h"
-#include "chrome/browser/extensions/external_pref_extension_provider.h"
+#include "chrome/browser/extensions/external_extension_provider_interface.h"
+#include "chrome/browser/extensions/external_extension_provider_impl.h"
+#include "chrome/browser/extensions/external_pref_extension_loader.h"
 #include "chrome/browser/extensions/pack_extension_job.cc"
 #include "chrome/browser/file_system/browser_file_system_helper.h"
 #include "chrome/browser/in_process_webkit/dom_storage_context.h"
@@ -125,10 +126,13 @@ static void AssertEqualExtents(ExtensionExtent* extent1,
 
 }  // namespace
 
-class MockExtensionProvider : public ExternalExtensionProvider {
+class MockExtensionProvider : public ExternalExtensionProviderInterface {
  public:
-  explicit MockExtensionProvider(Extension::Location location)
-    : location_(location), visit_count_(0) {}
+  explicit MockExtensionProvider(
+      VisitorInterface* visitor,
+      Extension::Location location)
+  : location_(location), visitor_(visitor), visit_count_(0) {
+  }
   virtual ~MockExtensionProvider() {}
 
   void UpdateOrAddExtension(const std::string& id,
@@ -142,16 +146,17 @@ class MockExtensionProvider : public ExternalExtensionProvider {
   }
 
   // ExternalExtensionProvider implementation:
-  virtual void VisitRegisteredExtension(Visitor* visitor) const {
+  virtual void VisitRegisteredExtension() const {
     visit_count_++;
     for (DataMap::const_iterator i = extension_map_.begin();
          i != extension_map_.end(); ++i) {
       scoped_ptr<Version> version;
       version.reset(Version::GetVersionFromString(i->second.first));
 
-      visitor->OnExternalExtensionFileFound(
+      visitor_->OnExternalExtensionFileFound(
           i->first, version.get(), i->second.second, location_);
     }
+    visitor_->OnExternalProviderReady();
   }
 
   virtual bool HasExtension(const std::string& id) const {
@@ -173,6 +178,14 @@ class MockExtensionProvider : public ExternalExtensionProvider {
 
     return true;
   }
+
+  virtual bool IsReady() {
+    return true;
+  }
+
+  virtual void ServiceShutdown() {
+  }
+
   int visit_count() const { return visit_count_; }
   void set_visit_count(int visit_count) {
     visit_count_ = visit_count;
@@ -182,6 +195,7 @@ class MockExtensionProvider : public ExternalExtensionProvider {
   typedef std::map< std::string, std::pair<std::string, FilePath> > DataMap;
   DataMap extension_map_;
   Extension::Location location_;
+  VisitorInterface* visitor_;
 
   // visit_count_ tracks the number of calls to VisitRegisteredExtension().
   // Mutable because it must be incremented on each call to
@@ -192,15 +206,19 @@ class MockExtensionProvider : public ExternalExtensionProvider {
   DISALLOW_COPY_AND_ASSIGN(MockExtensionProvider);
 };
 
-class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
+class MockProviderVisitor
+    : public ExternalExtensionProviderInterface::VisitorInterface {
  public:
   MockProviderVisitor() : ids_found_(0) {
   }
 
   int Visit(const std::string& json_data) {
     // Give the test json file to the provider for parsing.
-    provider_.reset(new ExternalPrefExtensionProvider());
-    provider_->SetPreferencesForTesting(json_data);
+    provider_.reset(new ExternalExtensionProviderImpl(
+        this,
+        new ExternalTestingExtensionLoader(json_data),
+        Extension::EXTERNAL_PREF,
+        Extension::EXTERNAL_PREF_DOWNLOAD));
 
     // We also parse the file into a dictionary to compare what we get back
     // from the provider.
@@ -219,7 +237,7 @@ class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
     // Reset our counter.
     ids_found_ = 0;
     // Ask the provider to look up all extensions and return them.
-    provider_->VisitRegisteredExtension(this);
+    provider_->VisitRegisteredExtension();
 
     return ids_found_;
   }
@@ -285,10 +303,14 @@ class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
     }
   }
 
+  virtual void OnExternalProviderReady() {
+    EXPECT_TRUE(provider_->IsReady());
+  }
+
  private:
   int ids_found_;
 
-  scoped_ptr<ExternalPrefExtensionProvider> provider_;
+  scoped_ptr<ExternalExtensionProviderImpl> provider_;
   scoped_ptr<DictionaryValue> prefs_;
 
   DISALLOW_COPY_AND_ASSIGN(MockProviderVisitor);
@@ -471,7 +493,7 @@ class ExtensionServiceTest
     }
   }
 
-  void AddMockExternalProvider(ExternalExtensionProvider* provider) {
+  void AddMockExternalProvider(ExternalExtensionProviderInterface* provider) {
     service_->AddProviderForTesting(provider);
   }
 
@@ -1071,8 +1093,10 @@ TEST_F(ExtensionServiceTest, KilledExtensions) {
   FilePath path = extensions_path.AppendASCII("good.crx");
   set_extensions_enabled(true);
 
+  scoped_ptr<Version> version;
+  version.reset(Version::GetVersionFromString("1.0.0.0"));
   // Install an external extension.
-  service_->OnExternalExtensionFileFound(good_crx, "1.0.0.0",
+  service_->OnExternalExtensionFileFound(good_crx, version.get(),
                                          path, Extension::EXTERNAL_PREF);
   loop_.RunAllPending();
   ASSERT_TRUE(NULL != service_->GetExtensionById(good_crx, false));
@@ -1083,15 +1107,16 @@ TEST_F(ExtensionServiceTest, KilledExtensions) {
   ValidateIntegerPref(good_crx, "location", Extension::KILLBIT);
 
   // Try to re-install it externally. This should fail because of the killbit.
-  service_->OnExternalExtensionFileFound(good_crx, "1.0.0.0",
+  service_->OnExternalExtensionFileFound(good_crx, version.get(),
                                          path, Extension::EXTERNAL_PREF);
   loop_.RunAllPending();
   ASSERT_TRUE(NULL == service_->GetExtensionById(good_crx, false));
   ValidateIntegerPref(good_crx, "location", Extension::KILLBIT);
 
+  version.reset(Version::GetVersionFromString("1.0.0.1"));
   // Repeat the same thing with a newer version of the extension.
   path = extensions_path.AppendASCII("good2.crx");
-  service_->OnExternalExtensionFileFound(good_crx, "1.0.0.1",
+  service_->OnExternalExtensionFileFound(good_crx, version.get(),
                                          path, Extension::EXTERNAL_PREF);
   loop_.RunAllPending();
   ASSERT_TRUE(NULL == service_->GetExtensionById(good_crx, false));
@@ -2862,8 +2887,7 @@ void ExtensionServiceTest::TestExternalProvider(
   provider->RemoveExtension(good_crx);
 
   loaded_.clear();
-  service_->UnloadAllExtensions();
-  service_->LoadAllExtensions();
+  service_->OnExternalProviderReady();
   loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
   ValidatePrefKeyCount(0);
@@ -2909,7 +2933,7 @@ TEST_F(ExtensionServiceTest, ExternalInstallRegistry) {
 
   // Now add providers. Extension system takes ownership of the objects.
   MockExtensionProvider* reg_provider =
-      new MockExtensionProvider(Extension::EXTERNAL_REGISTRY);
+      new MockExtensionProvider(service_.get(), Extension::EXTERNAL_REGISTRY);
   AddMockExternalProvider(reg_provider);
   TestExternalProvider(reg_provider, Extension::EXTERNAL_REGISTRY);
 }
@@ -2920,7 +2944,7 @@ TEST_F(ExtensionServiceTest, ExternalInstallPref) {
 
   // Now add providers. Extension system takes ownership of the objects.
   MockExtensionProvider* pref_provider =
-      new MockExtensionProvider(Extension::EXTERNAL_PREF);
+      new MockExtensionProvider(service_.get(), Extension::EXTERNAL_PREF);
 
   AddMockExternalProvider(pref_provider);
   TestExternalProvider(pref_provider, Extension::EXTERNAL_PREF);
@@ -2939,7 +2963,8 @@ TEST_F(ExtensionServiceTest, ExternalInstallPrefUpdateUrl) {
   // what the visitor does results in an extension being downloaded and
   // installed.
   MockExtensionProvider* pref_provider =
-      new MockExtensionProvider(Extension::EXTERNAL_PREF_DOWNLOAD);
+      new MockExtensionProvider(service_.get(),
+                                Extension::EXTERNAL_PREF_DOWNLOAD);
   AddMockExternalProvider(pref_provider);
   TestExternalProvider(pref_provider, Extension::EXTERNAL_PREF_DOWNLOAD);
 }
@@ -3119,7 +3144,7 @@ TEST(ExtensionServiceTestSimple, Enabledness) {
   // By default, we are enabled.
   command_line.reset(new CommandLine(CommandLine::NO_PROGRAM));
   service = profile->CreateExtensionService(command_line.get(),
-                                             install_dir);
+                                            install_dir);
   EXPECT_TRUE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();
@@ -3140,7 +3165,7 @@ TEST(ExtensionServiceTestSimple, Enabledness) {
   profile.reset(new TestingProfile());
   profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true);
   service = profile->CreateExtensionService(command_line.get(),
-                                             install_dir);
+                                            install_dir);
   EXPECT_FALSE(service->extensions_enabled());
   service->Init();
   loop.RunAllPending();