ExtensionSystem cleanup part 2

This change introduces separate ExtensionSystemFactories for app shell and for chrome; in particular, a new ShellExtensionSystemFactory. ExtensionsBrowserClient now selects between the two at the factory level. Callers can simply use ExtensionSystem::Get.

BUG=337707

Review URL: https://codereview.chromium.org/137753016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251014 0039d316-1c4b-4281-b951-d872f2087c98

apps/DEPS

@@ -32,10 +32,6 @@ include_rules = [
   "+chrome/browser/extensions/extension_icon_image.h",
   "+chrome/browser/extensions/extension_host.h",
   "+chrome/browser/extensions/extension_keybinding_registry.h",
-  "+chrome/browser/extensions/extension_prefs.h",
-  "+chrome/browser/extensions/extension_prefs_factory.h",
-  "+chrome/browser/extensions/extension_system.h",
-  "+chrome/browser/extensions/extension_system_factory.h",
   "+chrome/browser/extensions/extension_web_contents_observer.h",
   "+chrome/browser/extensions/suggest_permission_util.h",
   "+chrome/browser/extensions/unpacked_installer.h",

apps/app_load_service_factory.cc

@@ -6,10 +6,10 @@
 
 #include "apps/app_load_service.h"
 #include "apps/shell_window_registry.h"
-#include "chrome/browser/extensions/extension_system_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 #include "extensions/browser/extension_prefs_factory.h"
+#include "extensions/browser/extension_system_provider.h"
 #include "extensions/browser/extensions_browser_client.h"
 
 namespace apps {
@@ -28,8 +28,9 @@ AppLoadServiceFactory::AppLoadServiceFactory()
     : BrowserContextKeyedServiceFactory(
         "AppLoadService",
         BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(
+      extensions::ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
   DependsOn(extensions::ExtensionPrefsFactory::GetInstance());
-  DependsOn(extensions::ExtensionSystemFactory::GetInstance());
   DependsOn(ShellWindowRegistry::Factory::GetInstance());
 }
 

apps/apps.gypi

@@ -137,6 +137,8 @@
             'shell/browser/shell_content_browser_client.h',
             'shell/browser/shell_extension_system.cc',
             'shell/browser/shell_extension_system.h',
+            'shell/browser/shell_extension_system_factory.cc',
+            'shell/browser/shell_extension_system_factory.h',
             'shell/browser/shell_extensions_browser_client.cc',
             'shell/browser/shell_extensions_browser_client.h',
             'shell/browser/web_view_window.cc',

apps/shell/browser/shell_browser_main_parts.cc

@@ -6,6 +6,7 @@
 
 #include "apps/shell/browser/shell_browser_context.h"
 #include "apps/shell/browser/shell_extension_system.h"
+#include "apps/shell/browser/shell_extension_system_factory.h"
 #include "apps/shell/browser/shell_extensions_browser_client.h"
 #include "apps/shell/browser/web_view_window.h"
 #include "apps/shell/common/shell_extensions_client.h"
@@ -41,6 +42,7 @@ namespace {
 // ChromeBrowserMainExtraPartsProfiles for details.
 void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
   extensions::RendererStartupHelperFactory::GetInstance();
+  extensions::ShellExtensionSystemFactory::GetInstance();
 }
 
 // A ViewsDelegate to attach new unparented windows to app_shell's root window.
@@ -110,6 +112,7 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
 
   // Create our custom ExtensionSystem first because other
   // BrowserContextKeyedServices depend on it.
+  // TODO(yoz): Move this after EnsureBrowserContextKeyedServiceFactoriesBuilt.
   CreateExtensionSystem();
 
   EnsureBrowserContextKeyedServiceFactoriesBuilt();

apps/shell/browser/shell_extension_system.cc

@@ -17,9 +17,7 @@
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_prefs.h"
-#include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_registry_factory.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/browser/lazy_background_task_queue.h"
 #include "extensions/browser/process_manager.h"
@@ -38,15 +36,6 @@ ShellExtensionSystem::ShellExtensionSystem(BrowserContext* browser_context)
 ShellExtensionSystem::~ShellExtensionSystem() {
 }
 
-// static
-std::vector<BrowserContextKeyedServiceFactory*>
-ShellExtensionSystem::GetDependencies() {
-  std::vector<BrowserContextKeyedServiceFactory*> depends_on;
-  depends_on.push_back(ExtensionPrefsFactory::GetInstance());
-  depends_on.push_back(ExtensionRegistryFactory::GetInstance());
-  return depends_on;
-}
-
 bool ShellExtensionSystem::LoadAndLaunchApp(const base::FilePath& app_dir) {
   std::string load_error;
   scoped_refptr<Extension> extension =

apps/shell/browser/shell_extension_system.h

@@ -36,9 +36,6 @@ class ShellExtensionSystem : public ExtensionSystem {
   explicit ShellExtensionSystem(content::BrowserContext* browser_context);
   virtual ~ShellExtensionSystem();
 
-  // Returns the list of services on which |this| depends.
-  static std::vector<BrowserContextKeyedServiceFactory*> GetDependencies();
-
   // Loads an unpacked application from a directory and attempts to launch it.
   // Returns true on success.
   bool LoadAndLaunchApp(const base::FilePath& app_dir);

apps/shell/browser/shell_extension_system_factory.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "apps/shell/browser/shell_extension_system_factory.h"
+
+#include "apps/shell/browser/shell_extension_system.h"
+#include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
+#include "extensions/browser/extension_prefs_factory.h"
+#include "extensions/browser/extension_registry_factory.h"
+
+using content::BrowserContext;
+
+namespace extensions {
+
+ExtensionSystem* ShellExtensionSystemFactory::GetForBrowserContext(
+    BrowserContext* context) {
+  return static_cast<ShellExtensionSystem*>(
+      GetInstance()->GetServiceForBrowserContext(context, true));
+}
+
+// static
+ShellExtensionSystemFactory* ShellExtensionSystemFactory::GetInstance() {
+  return Singleton<ShellExtensionSystemFactory>::get();
+}
+
+ShellExtensionSystemFactory::ShellExtensionSystemFactory()
+    : ExtensionSystemProvider("ShellExtensionSystem",
+                              BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(ExtensionPrefsFactory::GetInstance());
+  DependsOn(ExtensionRegistryFactory::GetInstance());
+}
+
+ShellExtensionSystemFactory::~ShellExtensionSystemFactory() {}
+
+BrowserContextKeyedService*
+ShellExtensionSystemFactory::BuildServiceInstanceFor(BrowserContext* context)
+    const {
+  return new ShellExtensionSystem(context);
+}
+
+BrowserContext* ShellExtensionSystemFactory::GetBrowserContextToUse(
+    BrowserContext* context) const {
+  // Use a separate instance for incognito.
+  return context;
+}
+
+bool ShellExtensionSystemFactory::ServiceIsCreatedWithBrowserContext() const {
+  return true;
+}
+
+}  // namespace extensions

apps/shell/browser/shell_extension_system_factory.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef APPS_SHELL_BROWSER_SHELL_EXTENSION_SYSTEM_FACTORY_H_
+#define APPS_SHELL_BROWSER_SHELL_EXTENSION_SYSTEM_FACTORY_H_
+
+#include "base/memory/singleton.h"
+#include "extensions/browser/extension_system_provider.h"
+
+namespace extensions {
+
+// A factory that provides ShellExtensionSystem for app_shell.
+class ShellExtensionSystemFactory : public ExtensionSystemProvider {
+ public:
+  // ExtensionSystemProvider implementation:
+  virtual ExtensionSystem* GetForBrowserContext(
+      content::BrowserContext* context) OVERRIDE;
+
+  static ShellExtensionSystemFactory* GetInstance();
+
+ private:
+  friend struct DefaultSingletonTraits<ShellExtensionSystemFactory>;
+
+  ShellExtensionSystemFactory();
+  virtual ~ShellExtensionSystemFactory();
+
+  // BrowserContextKeyedServiceFactory implementation:
+  virtual BrowserContextKeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const OVERRIDE;
+  virtual content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const OVERRIDE;
+  virtual bool ServiceIsCreatedWithBrowserContext() const OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(ShellExtensionSystemFactory);
+};
+
+}  // namespace extensions
+
+#endif  // APPS_SHELL_BROWSER_SHELL_EXTENSION_SYSTEM_FACTORY_H_

apps/shell/browser/shell_extensions_browser_client.cc

@@ -5,7 +5,7 @@
 #include "apps/shell/browser/shell_extensions_browser_client.h"
 
 #include "apps/shell/browser/shell_app_sorting.h"
-#include "apps/shell/browser/shell_extension_system.h"
+#include "apps/shell/browser/shell_extension_system_factory.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/pref_service_factory.h"
 #include "base/prefs/testing_pref_store.h"
@@ -148,14 +148,9 @@ ApiActivityMonitor* ShellExtensionsBrowserClient::GetApiActivityMonitor(
   return NULL;
 }
 
-std::vector<BrowserContextKeyedServiceFactory*>
-ShellExtensionsBrowserClient::GetExtensionSystemDependencies() {
-  return ShellExtensionSystem::GetDependencies();
-}
-
-ExtensionSystem* ShellExtensionsBrowserClient::CreateExtensionSystem(
-    BrowserContext* context) {
-  return new ShellExtensionSystem(context);
+ExtensionSystemProvider*
+ShellExtensionsBrowserClient::GetExtensionSystemFactory() {
+  return ShellExtensionSystemFactory::GetInstance();
 }
 
 }  // namespace extensions

apps/shell/browser/shell_extensions_browser_client.h

@@ -57,10 +57,7 @@ class ShellExtensionsBrowserClient : public ExtensionsBrowserClient {
       OVERRIDE;
   virtual ApiActivityMonitor* GetApiActivityMonitor(
       content::BrowserContext* context) OVERRIDE;
-  virtual std::vector<BrowserContextKeyedServiceFactory*>
-      GetExtensionSystemDependencies() OVERRIDE;
-  virtual ExtensionSystem* CreateExtensionSystem(
-      content::BrowserContext* context) OVERRIDE;
+  virtual ExtensionSystemProvider* GetExtensionSystemFactory() OVERRIDE;
 
  private:
   // The single BrowserContext for app_shell. Not owned.

chrome/browser/extensions/chrome_extensions_browser_client.cc

@@ -203,16 +203,9 @@ ApiActivityMonitor* ChromeExtensionsBrowserClient::GetApiActivityMonitor(
   return ActivityLog::GetInstance(context);
 }
 
-std::vector<BrowserContextKeyedServiceFactory*>
-ChromeExtensionsBrowserClient::GetExtensionSystemDependencies() {
-  std::vector<BrowserContextKeyedServiceFactory*> dependencies;
-  dependencies.push_back(ExtensionSystemSharedFactory::GetInstance());
-  return dependencies;
-}
-
-ExtensionSystem* ChromeExtensionsBrowserClient::CreateExtensionSystem(
-    content::BrowserContext* context) {
-  return new ExtensionSystemImpl(static_cast<Profile*>(context));
+ExtensionSystemProvider*
+ChromeExtensionsBrowserClient::GetExtensionSystemFactory() {
+  return ExtensionSystemFactory::GetInstance();
 }
 
 }  // namespace extensions

chrome/browser/extensions/chrome_extensions_browser_client.h

@@ -68,10 +68,7 @@ class ChromeExtensionsBrowserClient : public ExtensionsBrowserClient {
       OVERRIDE;
   virtual ApiActivityMonitor* GetApiActivityMonitor(
       content::BrowserContext* context) OVERRIDE;
-  virtual std::vector<BrowserContextKeyedServiceFactory*>
-      GetExtensionSystemDependencies() OVERRIDE;
-  virtual ExtensionSystem* CreateExtensionSystem(
-      content::BrowserContext* context) OVERRIDE;
+  virtual ExtensionSystemProvider* GetExtensionSystemFactory() OVERRIDE;
 
  private:
   friend struct base::DefaultLazyInstanceTraits<ChromeExtensionsBrowserClient>;

chrome/browser/extensions/extension_system_factory.cc

@@ -73,15 +73,11 @@ ExtensionSystemFactory* ExtensionSystemFactory::GetInstance() {
 }
 
 ExtensionSystemFactory::ExtensionSystemFactory()
-    : BrowserContextKeyedServiceFactory(
-        "ExtensionSystem",
-        BrowserContextDependencyManager::GetInstance()) {
+    : ExtensionSystemProvider("ExtensionSystem",
+                              BrowserContextDependencyManager::GetInstance()) {
   DCHECK(ExtensionsBrowserClient::Get())
       << "ExtensionSystemFactory must be initialized after BrowserProcess";
-  std::vector<BrowserContextKeyedServiceFactory*> dependencies =
-      ExtensionsBrowserClient::Get()->GetExtensionSystemDependencies();
-  for (size_t i = 0; i < dependencies.size(); ++i)
-    DependsOn(dependencies[i]);
+  DependsOn(ExtensionSystemSharedFactory::GetInstance());
 }
 
 ExtensionSystemFactory::~ExtensionSystemFactory() {
@@ -89,7 +85,7 @@ ExtensionSystemFactory::~ExtensionSystemFactory() {
 
 BrowserContextKeyedService* ExtensionSystemFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return ExtensionsBrowserClient::Get()->CreateExtensionSystem(context);
+  return new ExtensionSystemImpl(static_cast<Profile*>(context));
 }
 
 content::BrowserContext* ExtensionSystemFactory::GetBrowserContextToUse(

chrome/browser/extensions/extension_system_factory.h

@@ -8,7 +8,7 @@
 #include "base/memory/singleton.h"
 #include "chrome/browser/extensions/extension_system_impl.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service_factory.h"
-#include "extensions/browser/extension_system.h"
+#include "extensions/browser/extension_system_provider.h"
 
 class BrowserContextKeyedService;
 
@@ -35,13 +35,17 @@ class ExtensionSystemSharedFactory : public BrowserContextKeyedServiceFactory {
       content::BrowserContext* context) const OVERRIDE;
   virtual content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionSystemSharedFactory);
 };
 
-// BrowserContextKeyedServiceFactory for ExtensionSystem.
-class ExtensionSystemFactory : public BrowserContextKeyedServiceFactory {
+// BrowserContextKeyedServiceFactory for ExtensionSystemImpl.
+// TODO(yoz): Rename to ExtensionSystemImplFactory.
+class ExtensionSystemFactory : public ExtensionSystemProvider {
  public:
-  static ExtensionSystem* GetForBrowserContext(
-      content::BrowserContext* context);
+  // ExtensionSystem provider implementation:
+  virtual ExtensionSystem* GetForBrowserContext(
+      content::BrowserContext* context) OVERRIDE;
 
   static ExtensionSystemFactory* GetInstance();
 
@@ -57,6 +61,8 @@ class ExtensionSystemFactory : public BrowserContextKeyedServiceFactory {
   virtual content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const OVERRIDE;
   virtual bool ServiceIsCreatedWithBrowserContext() const OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionSystemFactory);
 };
 
 }  // namespace extensions

extensions/browser/DEPS

@@ -18,7 +18,6 @@ include_rules = [
   "+chrome/browser/extensions/extension_function_histogram_value.h",
   "+chrome/browser/extensions/extension_host.h",
   "+chrome/browser/extensions/extension_service.h",
-  "+chrome/browser/extensions/extension_system_factory.h",
   "+chrome/browser/renderer_host/chrome_render_message_filter.h",
   "+chrome/common/extensions/extension_messages.h",
   "+chrome/common/extensions/features/feature_channel.h",

extensions/browser/extension_system.cc

@@ -4,7 +4,9 @@
 
 #include "extensions/browser/extension_system.h"
 
-#include "chrome/browser/extensions/extension_system_factory.h"
+#include "components/browser_context_keyed_service/browser_context_keyed_service_factory.h"
+#include "extensions/browser/extension_system_provider.h"
+#include "extensions/browser/extensions_browser_client.h"
 
 namespace extensions {
 
@@ -16,7 +18,9 @@ ExtensionSystem::~ExtensionSystem() {
 
 // static
 ExtensionSystem* ExtensionSystem::Get(content::BrowserContext* context) {
-  return ExtensionSystemFactory::GetForBrowserContext(context);
+  return ExtensionsBrowserClient::Get()
+      ->GetExtensionSystemFactory()
+      ->GetForBrowserContext(context);
 }
 
 }  // namespace extensions

extensions/browser/extension_system.h

@@ -51,7 +51,6 @@ class ExtensionSystem : public BrowserContextKeyedService {
   virtual ~ExtensionSystem();
 
   // Returns the instance for the given browser context, or NULL if none.
-  // A convenience wrapper around ExtensionSystemFactory::GetForBrowserContext.
   static ExtensionSystem* Get(content::BrowserContext* context);
 
   // Initializes extensions machinery.

extensions/browser/extension_system_provider.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/extension_system_provider.h"
+
+#include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
+
+namespace extensions {
+
+ExtensionSystemProvider::ExtensionSystemProvider(
+    const char* name,
+    BrowserContextDependencyManager* manager)
+    : BrowserContextKeyedServiceFactory(name, manager) {
+}
+
+ExtensionSystemProvider::~ExtensionSystemProvider() {}
+
+}  // namespace extensions

extensions/browser/extension_system_provider.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_EXTENSION_SYSTEM_PROVIDER_H_
+#define EXTENSIONS_BROWSER_EXTENSION_SYSTEM_PROVIDER_H_
+
+#include "components/browser_context_keyed_service/browser_context_keyed_service_factory.h"
+
+class BrowserContextDependencyManager;
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+class ExtensionSystem;
+
+// An ExtensionSystemProvider maps a BrowserContext to its ExtensionSystem.
+// Different applications may use this to provide differing implementations
+// of ExtensionSystem.
+// TODO(yoz): Rename to ExtensionSystemFactory.
+class ExtensionSystemProvider : public BrowserContextKeyedServiceFactory {
+ public:
+  ExtensionSystemProvider(const char* name,
+                          BrowserContextDependencyManager* manager);
+  virtual ~ExtensionSystemProvider();
+
+  virtual ExtensionSystem* GetForBrowserContext(
+      content::BrowserContext* context) = 0;
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_EXTENSION_SYSTEM_PROVIDER_H_

extensions/browser/extensions_browser_client.h

@@ -10,7 +10,6 @@
 
 #include "base/memory/scoped_ptr.h"
 
-class BrowserContextKeyedServiceFactory;
 class CommandLine;
 class PrefService;
 
@@ -27,6 +26,7 @@ class AppSorting;
 class Extension;
 class ExtensionHost;
 class ExtensionSystem;
+class ExtensionSystemProvider;
 
 // Interface to allow the extensions module to make browser-process-specific
 // queries of the embedder. Should be Set() once in the browser process.
@@ -122,13 +122,9 @@ class ExtensionsBrowserClient {
   virtual ApiActivityMonitor* GetApiActivityMonitor(
       content::BrowserContext* context) = 0;
 
-  // Returns the dependencies of ExtensionSystem. May return an empty list.
-  virtual std::vector<BrowserContextKeyedServiceFactory*>
-      GetExtensionSystemDependencies() = 0;
-
-  // Creates a new ExtensionSystem for |context|.
-  virtual ExtensionSystem* CreateExtensionSystem(
-      content::BrowserContext* context) = 0;
+  // Returns the factory that provides an ExtensionSystem to be returned from
+  // ExtensionSystem::Get.
+  virtual ExtensionSystemProvider* GetExtensionSystemFactory() = 0;
 
   // Returns the single instance of |this|.
   static ExtensionsBrowserClient* Get();

extensions/extensions.gyp

@@ -201,6 +201,8 @@
         'browser/extension_scoped_prefs.h',
         'browser/extension_system.cc',
         'browser/extension_system.h',
+        'browser/extension_system_provider.cc',
+        'browser/extension_system_provider.h',
         'browser/extensions_browser_client.cc',
         'browser/extensions_browser_client.h',
         'browser/external_provider_interface.h',