Move extensions BCKS factory creation

Move extensions BCKS factory creation

The list of BCKS factories being poked by ChromeBrowserMainExtraPartsProfiles
is quite long and extensions code is the biggest contributor to that length.

This moves extensions-related factory references into three new modules
(one for //apps, one for //extensions, and one for //c/b/extensions) and
condenses Chrome's extensions-related factory creation down to three calls
within ChromeBrowserMainExtraPartsProfiles.

BUG=352399
TBR=erg@chromium.org (for profiles parts)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257936

[Ken Rockot(use gerrit already), 2014-03-15 04:29:23]

I think this is worth doing, but I'm not terribly happy with it yet:

1. Wow that's a long function name.

2. I wish more could be done to clean up BCKS building in general. The giant
pile-o-calls is so bad, but I don't have any better ideas that don't depend on
evil and illegal things like static initialization.

[James Cook, 2014-03-17 17:31:50]

I like this change.

(Also, in general I like lists of things to initialize instead of magic things
with static initializers. I've tried to use magic in the past and it *almost*
always works. And when it doesn't it's a nightmare.)

https://codereview.chromium.org/195943018/diff/40001/apps/browser_context_key...
File apps/browser_context_keyed_service_factories.h (right):

https://codereview.chromium.org/195943018/diff/40001/apps/browser_context_key...
apps/browser_context_keyed_service_factories.h:11: // the core extensions code.
nit: core apps code?

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc
(right):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:102:
extensions::ScreenlockPrivateEventRouter::GetFactoryInstance();
nit: Alphabetize this one?

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:117:
#endif  // defined(OS_CHROMEOS) || defined(OS_WIN)
nit: Comment doesn't match #if. I would just remove the comment.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:141:
file_manager::FileBrowserPrivateAPIFactory::GetInstance();
Just double-checking: This one wasn't wrapped in ENABLE_EXTENSIONS before. Are
you sure it's OK to move?

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:145:
extensions::ApiResourceManager<
Any particular reason these aren't alphabetized? I thought you missed them when
I reviewed the list above.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:57: #include
"chrome/browser/extensions/activity_log/activity_log.h"
In the old code, headers like this were not included/compiled if
ENABLE_EXTENSIONS wasn't set.  In the new code, I think they are.  Can
apps/browser_context_keyed_service_factories.{h|cc} be excluded if
enable_extensions==0?

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:53: #include
"apps/browser_context_keyed_service_factories.h"
It's kind of weird that these three files all have different names. I would
either call them all browser_context_keyed_service_factories.h, or if MSVC/GYP
can't handle that, call them apps_browser_context_keyed_service_factories,
chrome_browser..., extensions_browser...

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:137:
AppShortcutManagerFactory::GetInstance();
These used to be wrapped in ENABLE_EXTENSIONS. Do they need to always be in the
build?

Also, I would move these below the ENABLE_EXTENSIONS block below.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:142:
EnsureChromeExtensionsBrowserContextKeyedServiceFactoriesBuilt();
Ugh. Hmm. Yuck.

If it has to be an awful name like this, what about
EnsureBrowserContextKeyedServiceFactoriesBuiltForExtensions()? That was someone
grepping the code for "EnsureBrowserContextKeyed..." will find all the calls.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:143: #endif
nit: Put a blank line after this ENABLE_EXTENSIONS block to emphasize that this
section is slightly different than what comes afterward.

https://codereview.chromium.org/195943018/diff/40001/extensions/browser/conte...
File extensions/browser/context_keyed_service_factories.cc (right):

https://codereview.chromium.org/195943018/diff/40001/extensions/browser/conte...
extensions/browser/context_keyed_service_factories.cc:31:
extensions::SettingsFrontend::GetFactoryInstance();
head's up, Devlin renamed SettingsFrontend to StorageFrontend so you'll need to
rebase

[Ken Rockot(use gerrit already), 2014-03-17 20:08:26]

Thanks. Moar changes.

https://codereview.chromium.org/195943018/diff/40001/apps/browser_context_key...
File apps/browser_context_keyed_service_factories.h (right):

https://codereview.chromium.org/195943018/diff/40001/apps/browser_context_key...
apps/browser_context_keyed_service_factories.h:11: // the core extensions code.
On 2014/03/17 17:31:51, James Cook wrote:
> nit: core apps code?

Done.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc
(right):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:102:
extensions::ScreenlockPrivateEventRouter::GetFactoryInstance();
On 2014/03/17 17:31:51, James Cook wrote:
> nit: Alphabetize this one?

Done.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:117:
#endif  // defined(OS_CHROMEOS) || defined(OS_WIN)
On 2014/03/17 17:31:51, James Cook wrote:
> nit: Comment doesn't match #if. I would just remove the comment.

Done.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:141:
file_manager::FileBrowserPrivateAPIFactory::GetInstance();
On 2014/03/17 17:31:51, James Cook wrote:
> Just double-checking: This one wasn't wrapped in ENABLE_EXTENSIONS before. Are
> you sure it's OK to move?

Not 100% sure, but I couldn't convince myself that it wasn't OK. Since I'd like
to live in a world where a dependency on an extensions API implies a dependency
on ENABLE_EXTENSIONS, I thought I'd give it a shot. :)

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_context_keyed_service_factories.cc:145:
extensions::ApiResourceManager<
On 2014/03/17 17:31:51, James Cook wrote:
> Any particular reason these aren't alphabetized? I thought you missed them
when
> I reviewed the list above.

Just an oversight on my part. Moved.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (left):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:57: #include
"chrome/browser/extensions/activity_log/activity_log.h"
On 2014/03/17 17:31:51, James Cook wrote:
> In the old code, headers like this were not included/compiled if
> ENABLE_EXTENSIONS wasn't set.  In the new code, I think they are.  Can
> apps/browser_context_keyed_service_factories.{h|cc} be excluded if
> enable_extensions==0?

Done.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:53: #include
"apps/browser_context_keyed_service_factories.h"
On 2014/03/17 17:31:51, James Cook wrote:
> It's kind of weird that these three files all have different names. I would
> either call them all browser_context_keyed_service_factories.h, or if MSVC/GYP
> can't handle that, call them apps_browser_context_keyed_service_factories,
> chrome_browser..., extensions_browser...

Yeah, I didn't want to leave them like this. All changed to
browser_context_keyed_service_factories pending verification that MSVC doesn't
choke. I think if any of the affected targets builds more than one of these
modules, we're Doing It Wrong.

I decided against "keyed_service_factories" after all since it felt like I was
just setting someone  else up to rename it in the future. :)

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:137:
AppShortcutManagerFactory::GetInstance();
On 2014/03/17 17:31:51, James Cook wrote:
> These used to be wrapped in ENABLE_EXTENSIONS. Do they need to always be in
the
> build?
> 
> Also, I would move these below the ENABLE_EXTENSIONS block below.

Moved into the ENABLE_EXTENSIONS block.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:142:
EnsureChromeExtensionsBrowserContextKeyedServiceFactoriesBuilt();
On 2014/03/17 17:31:51, James Cook wrote:
> Ugh. Hmm. Yuck.
> 
> If it has to be an awful name like this, what about
> EnsureBrowserContextKeyedServiceFactoriesBuiltForExtensions()? That was
someone
> grepping the code for "EnsureBrowserContextKeyed..." will find all the calls.

How about consistent function names
(EnsureBrowserContextKeyedServiceFactoriesBuilt) each in their own namespace?
apps::, extensions::, and a new chrome_extensions::

The latter would be newly introduced, but any extensions code under //chrome
should ultimately wind up within it, kind of like chrome-only API code should
eventually wind up in chrome_api:: or perhaps even chrome_extensions::api...
Bah, I digress.

https://codereview.chromium.org/195943018/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:143: #endif
On 2014/03/17 17:31:51, James Cook wrote:
> nit: Put a blank line after this ENABLE_EXTENSIONS block to emphasize that
this
> section is slightly different than what comes afterward.

Done.

https://codereview.chromium.org/195943018/diff/40001/extensions/browser/conte...
File extensions/browser/context_keyed_service_factories.cc (right):

https://codereview.chromium.org/195943018/diff/40001/extensions/browser/conte...
extensions/browser/context_keyed_service_factories.cc:31:
extensions::SettingsFrontend::GetFactoryInstance();
On 2014/03/17 17:31:51, James Cook wrote:
> head's up, Devlin renamed SettingsFrontend to StorageFrontend so you'll need
to
> rebase

Done.

[James Cook, 2014-03-17 23:56:59]

LGTM with one nitty nit.

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/browser_context_keyed_service_factories.cc
(right):

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/extension...
chrome/browser/extensions/browser_context_keyed_service_factories.cc:147:
file_manager::FileBrowserPrivateAPIFactory::GetInstance();
On more reflection this should be OK because Chrome OS always has extensions
enabled.

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:141:
apps::EnsureBrowserContextKeyedServiceFactoriesBuilt();
super-nitty-nit: I would move these three lines above the
AppShortcutManagerFactory, just to emphasize doing general-extensions stuff
first, then a list of specific things (alphabetized) afterwards.

[Ken Rockot(use gerrit already), 2014-03-18 00:29:50]

done

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/195943018/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:141:
apps::EnsureBrowserContextKeyedServiceFactoriesBuilt();
On 2014/03/17 23:56:59, James Cook wrote:
> super-nitty-nit: I would move these three lines above the
> AppShortcutManagerFactory, just to emphasize doing general-extensions stuff
> first, then a list of specific things (alphabetized) afterwards.

Done.

[Yoyo Zhou, 2014-03-18 00:54:28]

LGTM
There's no real way around declaring a list of things *somewhere*. I think I
previously objected to splitting this list because 1 list would be better than 4
if you're looking for something in the list, but this list is more of a
declare-and-forget-about list.

[Ken Rockot(use gerrit already), 2014-03-18 00:55:57]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-18 00:56:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/80001

[commit-bot: I haz the power, 2014-03-18 01:04:22]

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

[commit-bot: I haz the power, 2014-03-18 01:04:23]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Ken Rockot(use gerrit already), 2014-03-18 17:42:52]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:46:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/100001

[Ken Rockot(use gerrit already), 2014-03-18 20:21:53]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:32:22]

Failed to trigger a try job on mac_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-03-18 20:33:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/120001

[commit-bot: I haz the power, 2014-03-18 20:55:19]

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

[commit-bot: I haz the power, 2014-03-18 20:55:19]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[Ken Rockot(use gerrit already), 2014-03-18 21:54:25]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/140001

[commit-bot: I haz the power, 2014-03-18 23:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/140001

[commit-bot: I haz the power, 2014-03-19 01:11:38]

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

[commit-bot: I haz the power, 2014-03-19 01:11:39]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Ken Rockot(use gerrit already), 2014-03-19 01:24:34]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/160001

[commit-bot: I haz the power, 2014-03-19 01:30:01]

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

[commit-bot: I haz the power, 2014-03-19 01:30:02]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Ken Rockot(use gerrit already), 2014-03-19 01:57:43]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:58:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/160001

[commit-bot: I haz the power, 2014-03-19 03:06:31]

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

[commit-bot: I haz the power, 2014-03-19 03:06:32]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ken Rockot(use gerrit already), 2014-03-19 03:12:27]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-19 03:12:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/195943018/160001

[commit-bot: I haz the power, 2014-03-19 11:50:12]

Change committed as 257936