Ensure the ExtensionSystem is ready before initializing an ExtensionAppModelBuilder

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 78fad9ec0e7f65f84334d8e7cd8eb2e559f09a84..ea5d0a6eef20d107e28cb3db5aee613ef1433a73 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -478,6 +478,18 @@ ExtensionServiceTestBase::~ExtensionServiceTestBase() {
 
 void ExtensionServiceTestBase::InitializeExtensionService(
     const ExtensionServiceTestBase::ExtensionServiceInitParams& params) {
+  InitializeExtensionServiceForProfile(params, &profile_, &service_);
+  management_policy_ =
+      ExtensionSystem::Get(profile_.get())->management_policy();
+  extensions_install_dir_ = params.extensions_install_dir;
+  expected_extensions_count_ = 0;
+}
+
+// static
+void ExtensionServiceTestBase::InitializeExtensionServiceForProfile(
+    const ExtensionServiceInitParams& params,
+    scoped_ptr<TestingProfile>* profile_ptr,

[stevenjb, 2013/11/20 19:41:15]

This is pretty confusing. Use of scoped_ptr<>& and *& below is especially
confusing. I think we should break this up into a function that creates the
profile and returns a scoped_ptr<TestingProfile>, and a function that creates,
initializes, and returns the ExtensionService, i.e. something like:

profile_ = CreateTestingProfile(params);
service_ = InitializeExtensionServiceForProfile(params, profile_.get());

[benwells, 2013/11/21 00:47:47]

Yep, seems like you could encapsulate the bits you need in a more clean way.

[tapted, 2013/11/21 03:29:56]

Done.

+    ExtensionService** service_ptr) {
   TestingProfile::Builder profile_builder;
   // Create a PrefService that only contains user defined preference values.
   PrefServiceMockFactory factory;
@@ -498,10 +510,11 @@ void ExtensionServiceTestBase::InitializeExtensionService(
     profile_builder.SetManagedUserId("asdf");
 
   profile_builder.SetPath(params.profile_path);
-  profile_ = profile_builder.Build();
+  scoped_ptr<TestingProfile>& profile = *profile_ptr;
+  profile = profile_builder.Build();
 
   TestExtensionSystem* system = static_cast<TestExtensionSystem*>(
-      ExtensionSystem::Get(profile_.get()));
+      ExtensionSystem::Get(profile.get()));
   if (!params.is_first_run) {
     ExtensionPrefs* prefs = system->CreateExtensionPrefs(
         CommandLine::ForCurrentProcess(),
@@ -509,34 +522,28 @@ void ExtensionServiceTestBase::InitializeExtensionService(
     prefs->SetAlertSystemFirstRun();
   }
 
-  service_ = system->CreateExtensionService(
+  ExtensionService*& service = *service_ptr;
+  service = system->CreateExtensionService(

[benwells, 2013/11/21 00:47:47]

huh? Why so many *'s and &'s??
more importantly, why assign twice in a row?

[tapted, 2013/11/21 03:29:56]

All fixed :) - at this stage I was still aiming on getting bots to report in..

       CommandLine::ForCurrentProcess(),
       params.extensions_install_dir,
       params.autoupdate_enabled);
 
-  service_->SetFileTaskRunnerForTesting(
+  service->SetFileTaskRunnerForTesting(
       base::MessageLoopProxy::current().get());
-  service_->set_extensions_enabled(true);
-  service_->set_show_extensions_prompts(false);
-  service_->set_install_updates_when_idle_for_test(false);
-
-  management_policy_ =
-      ExtensionSystem::Get(profile_.get())->management_policy();
-
-  extensions_install_dir_ = params.extensions_install_dir;
+  service->set_extensions_enabled(true);
+  service->set_show_extensions_prompts(false);
+  service->set_install_updates_when_idle_for_test(false);
 
   // When we start up, we want to make sure there is no external provider,
   // since the ExtensionService on Windows will use the Registry as a default
   // provider and if there is something already registered there then it will
   // interfere with the tests. Those tests that need an external provider
   // will register one specifically.
-  service_->ClearProvidersForTesting();
+  service->ClearProvidersForTesting();
 
 #if defined(OS_CHROMEOS)
-  extensions::InstallLimiter::Get(profile_.get())->DisableForTest();
+  extensions::InstallLimiter::Get(profile.get())->DisableForTest();
 #endif
-
-  expected_extensions_count_ = 0;
 }
 
 void ExtensionServiceTestBase::InitializeInstalledExtensionService(
@@ -612,9 +619,16 @@ void ExtensionServiceTestBase::TearDown() {
 
 ExtensionServiceTestBase::ExtensionServiceInitParams
 ExtensionServiceTestBase::CreateDefaultInitParams() {
+  return CreateDefaultInitParamsInTempDir(&temp_dir_);
+}
+
+// static
+ExtensionServiceTestBase::ExtensionServiceInitParams
+ExtensionServiceTestBase::CreateDefaultInitParamsInTempDir(
+    base::ScopedTempDir* temp_dir_) {
   ExtensionServiceInitParams params;
-  EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
-  base::FilePath path = temp_dir_.path();
+  EXPECT_TRUE(temp_dir_->CreateUniqueTempDir());
+  base::FilePath path = temp_dir_->path();
   path = path.Append(FILE_PATH_LITERAL("TestingExtensionsPath"));
   EXPECT_TRUE(base::DeleteFile(path, true));
   base::PlatformFileError error = base::PLATFORM_FILE_OK;