ExtensionSystem cleanup part 2

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251014

[James Cook, 2014-02-12 17:18:11]

LGTM with nits. I like it much better than what I did.

https://codereview.chromium.org/137753016/diff/60001/apps/DEPS
File apps/DEPS (left):

https://codereview.chromium.org/137753016/diff/60001/apps/DEPS#oldcode38
apps/DEPS:38: "+chrome/browser/extensions/extension_system_factory.h",
w00t!

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_extension_system_factory.h (right):

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_extension_system_factory.h:16: virtual ExtensionSystem*
GetForBrowserContext(
nit: comment which interface you are overriding

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_extension_system_factory.h:32: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/137753016/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_factory.h (right):

https://codereview.chromium.org/137753016/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_system_factory.h:44: virtual
ExtensionSystem* GetForBrowserContext(
nit: comment which interface you are overriding

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/DEPS
File extensions/browser/DEPS (left):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/DEPS#...
extensions/browser/DEPS:21:
"+chrome/browser/extensions/extension_system_factory.h",
double w00t!

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
File extensions/browser/extension_system.cc (right):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
extensions/browser/extension_system.cc:21: return
static_cast<ExtensionSystem*>(ExtensionsBrowserClient::Get()
Does this need the static_cast?

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
File extensions/browser/extension_system_provider.h (right):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
extensions/browser/extension_system_provider.h:22: // TODO(yoz): Rename to
ExtensionSystemFactory.
I presume these renames are for step 3 or 4 or whatever?

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:124: virtual
ExtensionSystemProvider* GetExtensionSystemFactory() = 0;
Nice, I like this approach.

[Yoyo Zhou, 2014-02-12 23:32:10]

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_extension_system_factory.h (right):

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_extension_system_factory.h:16: virtual ExtensionSystem*
GetForBrowserContext(
On 2014/02/12 17:18:12, James Cook wrote:
> nit: comment which interface you are overriding

Done.

https://codereview.chromium.org/137753016/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_extension_system_factory.h:32: };
On 2014/02/12 17:18:12, James Cook wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/137753016/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_factory.h (right):

https://codereview.chromium.org/137753016/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_system_factory.h:44: virtual
ExtensionSystem* GetForBrowserContext(
On 2014/02/12 17:18:12, James Cook wrote:
> nit: comment which interface you are overriding

Done.

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
File extensions/browser/extension_system.cc (right):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
extensions/browser/extension_system.cc:21: return
static_cast<ExtensionSystem*>(ExtensionsBrowserClient::Get()
On 2014/02/12 17:18:12, James Cook wrote:
> Does this need the static_cast?

I guess not!

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
File extensions/browser/extension_system_provider.h (right):

https://codereview.chromium.org/137753016/diff/60001/extensions/browser/exten...
extensions/browser/extension_system_provider.h:22: // TODO(yoz): Rename to
ExtensionSystemFactory.
On 2014/02/12 17:18:12, James Cook wrote:
> I presume these renames are for step 3 or 4 or whatever?

Yeah, step 3 is to give things more correct names. Not sure if there is a step 4
- I was going to have one, but see note.

https://codereview.chromium.org/137753016/diff/140001/apps/app_load_service_f...
File apps/app_load_service_factory.cc (right):

https://codereview.chromium.org/137753016/diff/140001/apps/app_load_service_f...
apps/app_load_service_factory.cc:32:
extensions::ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
Note: I realized that I'll need to make this change for anyone who DependsOn
ExtensionSystemFactory right now, so that they use the correct
ExtensionSystemProvider.

(Currently in progress.)

[Yoyo Zhou, 2014-02-13 01:39:26]

Unfortunately, fixing ExtensionSystemFactory::GetInstance calls is shaping up to
be one of those refactors that touches dozens of files, so I'll do that
separately. chrome should be fine but app_shell could be broken in the meantime.

[Yoyo Zhou, 2014-02-13 01:39:39]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-02-13 01:42:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/137753016/220001

[commit-bot: I haz the power, 2014-02-13 02:22:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:22:04]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Yoyo Zhou, 2014-02-13 07:38:53]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-02-13 07:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/137753016/220001

[commit-bot: I haz the power, 2014-02-13 15:15:54]

Change committed as 251014