Refactor code that defers extension background page loading

extensions/browser/process_manager_unittest.cc

Index: extensions/browser/process_manager_unittest.cc
diff --git a/extensions/browser/process_manager_unittest.cc b/extensions/browser/process_manager_unittest.cc
index b72b1125a7f733302595108efb37833c8e9d3e64..a0a8a7aa716eb3bfa87a49e77bbc6409d135e73a 100644
--- a/extensions/browser/process_manager_unittest.cc
+++ b/extensions/browser/process_manager_unittest.cc
@@ -5,11 +5,16 @@
 #include "extensions/browser/process_manager.h"
 
 #include "chrome/browser/chrome_notification_types.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/test/test_browser_context.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/extension_system_provider.h"
 #include "extensions/browser/test_extensions_browser_client.h"
+#include "extensions/common/extension_set.h"
+#include "extensions/common/one_shot_event.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserContext;
@@ -33,12 +38,99 @@ class TestBrowserContextIncognito : public TestBrowserContext {
   DISALLOW_COPY_AND_ASSIGN(TestBrowserContextIncognito);
 };
 
+// An ExtensionSystem that only provides a ready signal.
+class MockExtensionSystem : public ExtensionSystem {
+ public:
+  MockExtensionSystem() {}
+  virtual ~MockExtensionSystem() {}
+
+  // ExtensionSystem overrides:
+  virtual void InitForRegularProfile(bool extensions_enabled) OVERRIDE {}
+  virtual ExtensionService* extension_service() OVERRIDE { return NULL; }
+  virtual RuntimeData* runtime_data() OVERRIDE { return NULL; }
+  virtual ManagementPolicy* management_policy() OVERRIDE { return NULL; }
+  virtual UserScriptMaster* user_script_master() OVERRIDE { return NULL; }
+  virtual ProcessManager* process_manager() OVERRIDE { return NULL; }
+  virtual StateStore* state_store() OVERRIDE { return NULL; }
+  virtual StateStore* rules_store() OVERRIDE { return NULL; }
+  virtual InfoMap* info_map() OVERRIDE { return NULL; }
+  virtual LazyBackgroundTaskQueue* lazy_background_task_queue() OVERRIDE {
+    return NULL;
+  }
+  virtual EventRouter* event_router() OVERRIDE { return NULL; }
+  virtual ExtensionWarningService* warning_service() OVERRIDE { return NULL; }
+  virtual Blacklist* blacklist() OVERRIDE { return NULL; }
+  virtual ErrorConsole* error_console() OVERRIDE { return NULL; }
+  virtual InstallVerifier* install_verifier() OVERRIDE { return NULL; }
+  virtual QuotaService* quota_service() OVERRIDE { return NULL; }
+  virtual const OneShotEvent& ready() const OVERRIDE { return ready_; }
+  virtual ContentVerifier* content_verifier() OVERRIDE { return NULL; }
+  virtual scoped_ptr<ExtensionSet> GetDependentExtensions(
+      const Extension* extension) OVERRIDE {
+    return scoped_ptr<ExtensionSet>();
+  }
+
+  void SetReady() {
+    ready_.Signal();
+  }
+
+ private:
+  OneShotEvent ready_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockExtensionSystem);
+};
+
+// An ExtensionSystemProvider for the MockExtensionSystem.
+class MockExtensionSystemFactory : public ExtensionSystemProvider {

[Yoyo Zhou, 2014/07/11 01:51:53]

Is it possible to use SetTestingFactoryAndUse? You *might* be able to avoid some
of this boilerplate.

[James Cook, 2014/07/11 21:19:29]

It turns out I don't need any of this.

+ public:
+  // ExtensionSystemProvider implementation:
+  virtual ExtensionSystem* GetForBrowserContext(
+      content::BrowserContext* context) OVERRIDE {
+    return static_cast<ExtensionSystem*>(
+        GetInstance()->GetServiceForBrowserContext(context, true));
+  }
+
+  static MockExtensionSystemFactory* GetInstance() {
+    return Singleton<MockExtensionSystemFactory>::get();
+  }
+
+ private:
+  friend struct DefaultSingletonTraits<MockExtensionSystemFactory>;
+
+  MockExtensionSystemFactory()
+      : ExtensionSystemProvider(
+            "MockExtensionSystem",
+            BrowserContextDependencyManager::GetInstance()) {}
+
+  virtual ~MockExtensionSystemFactory() {}
+
+  // BrowserContextKeyedServiceFactory implementation:
+  virtual KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const OVERRIDE {
+    return new MockExtensionSystem;
+  }
+
+  virtual content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const OVERRIDE {
+    // Use a separate instance for incognito.
+    return context;
+  }
+
+  virtual bool ServiceIsCreatedWithBrowserContext() const OVERRIDE {
+    return true;
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(MockExtensionSystemFactory);
+};
+
 }  // namespace
 
 class ProcessManagerTest : public testing::Test {
  public:
   ProcessManagerTest() : extensions_browser_client_(&original_context_) {
     extensions_browser_client_.SetIncognitoContext(&incognito_context_);
+    extensions_browser_client_.set_extension_system_factory(
+        MockExtensionSystemFactory::GetInstance());
     ExtensionsBrowserClient::Set(&extensions_browser_client_);
   }
 
@@ -48,6 +140,23 @@ class ProcessManagerTest : public testing::Test {
 
   BrowserContext* original_context() { return &original_context_; }
   BrowserContext* incognito_context() { return &incognito_context_; }
+  TestExtensionsBrowserClient* extensions_browser_client() {
+    return &extensions_browser_client_;
+  }
+
+  virtual void SetUp() OVERRIDE {
+    BrowserContextDependencyManager::GetInstance()->
+        CreateBrowserContextServicesForTest(&original_context_);
+    BrowserContextDependencyManager::GetInstance()->
+        CreateBrowserContextServicesForTest(&incognito_context_);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    BrowserContextDependencyManager::GetInstance()->
+        DestroyBrowserContextServices(&incognito_context_);
+    BrowserContextDependencyManager::GetInstance()->
+        DestroyBrowserContextServices(&original_context_);
+  }
 
   // Returns true if the notification |type| is registered for |manager| with
   // source |context|. Pass NULL for |context| for all sources.
@@ -125,6 +234,84 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) {
                            incognito_context()));
 }
 
+// Test that startup background hosts can be created when the browser context is
+// created before the extension system is ready (the usual order).
+//
+// NOTE: This test does not try to create real ExtensionsHosts because
+// ExtensionHost is tightly coupled to WebContents and can't be constructed in
+// unit tests.
+TEST_F(ProcessManagerTest, StartupBrowserContextFirst) {
+  scoped_ptr<ProcessManager> manager(
+      ProcessManager::Create(original_context()));
+  ASSERT_FALSE(manager->startup_background_hosts_created_for_test());
+
+  // Simulate the BrowserContext load completing.
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_PROFILE_CREATED,

[Yoyo Zhou, 2014/07/11 01:51:53]

We're trying to avoid this in src/extensions, right?

[James Cook, 2014/07/11 21:19:29]

Yes, this was the initial "test driven development" patch, trying to capture the
behavior of the system before I modified it. Now that notification is expunged
from src/extensions.

+      content::Source<BrowserContext>(original_context()),
+      content::NotificationService::NoDetails());
+
+  // Simulate the extension system becoming ready. Background hosts should be
+  // created.
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSIONS_READY,
+      content::Source<BrowserContext>(original_context()),
+      content::NotificationService::NoDetails());
+  EXPECT_TRUE(manager->startup_background_hosts_created_for_test());
+}
+
+// Test that startup background hosts can be created if the extension system is
+// ready before the browser context (due to an abnormally slow async load).
+TEST_F(ProcessManagerTest, StartupExtensionsReadyFirst) {
+  // Pretend the context is still loading.
+  extensions_browser_client()->set_defer_loading_background_hosts(true);

[Yoyo Zhou, 2014/07/11 01:51:53]

I think this is an issue. If we set_defer_loading_background_hosts(true), that
alone prevents background hosts from being created. So it's hard to tell whether
the other stuff you're doing (sending EXTENSIONS_READY in this test) has any
effect on the expectation in 279. Does that make sense?

[James Cook, 2014/07/11 21:19:29]

This should be clearer now. From the extensions module's perspective, the
embedder can defer background host creation for any reason and it can request
that background hosts be created whenever something interesting happens.

+
+  scoped_ptr<ProcessManager> manager(
+      ProcessManager::Create(original_context()));
+  ASSERT_FALSE(manager->startup_background_hosts_created_for_test());
+
+  // Simulate the extension system becoming ready. Should not create hosts
+  // because the BrowserContext isn't valid.
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSIONS_READY,
+      content::Source<BrowserContext>(original_context()),
+      content::NotificationService::NoDetails());
+  EXPECT_FALSE(manager->startup_background_hosts_created_for_test());
+
+  // Simulate the BrowserContext load completing. Now hosts should be created.
+  extensions_browser_client()->set_defer_loading_background_hosts(false);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_PROFILE_CREATED,
+      content::Source<BrowserContext>(original_context()),
+      content::NotificationService::NoDetails());
+  EXPECT_TRUE(manager->startup_background_hosts_created_for_test());
+}
+
+// Test that an extension embedder can defer extension host creation initially,
+// then have the hosts created when a browser window opens. (Chrome does this
+// when it is launched to show the app list)
+TEST_F(ProcessManagerTest, DeferUntilBrowserWindowReady) {

[Yoyo Zhou, 2014/07/11 01:51:53]

Do you also want a test in the opposite order (browser window ready, then
extension system ready?)

[James Cook, 2014/07/11 21:19:28]

More tests added.

+  extensions_browser_client()->set_defer_loading_background_hosts(true);
+
+  scoped_ptr<ProcessManager> manager(
+      ProcessManager::Create(original_context()));
+  ASSERT_FALSE(manager->startup_background_hosts_created_for_test());
+
+  // Extension system ready but not yet browser window.
+  static_cast<MockExtensionSystem*>(ExtensionSystem::Get(original_context()))
+      ->SetReady();
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSIONS_READY,
+      content::Source<BrowserContext>(original_context()),
+      content::NotificationService::NoDetails());
+  EXPECT_FALSE(manager->startup_background_hosts_created_for_test());
+
+  // Browser window opens.
+  extensions_browser_client()->set_defer_loading_background_hosts(false);
+  manager->OnBrowserWindowReady();
+  EXPECT_TRUE(manager->startup_background_hosts_created_for_test());
+}
+
 // Test that extensions get grouped in the right SiteInstance (and therefore
 // process) based on their URLs.
 TEST_F(ProcessManagerTest, ProcessGrouping) {