Extract ProcessManager from ExtensionSystem.

Extract ProcessManager from ExtensionSystem.

This change removes ProcessManager's //chrome dependency because while
ProcessManager and ExtensionSystem are in //extensions the
ExtensionSystemFactory is in //chrome. This allows a service in
//extensions to declare a dependency on the ProcessManager's factory
directly.

Most of this patch is changing,
extensions::ExtensionSystem::Get(profile_)->process_manager() to, extensions::ProcessManager::Get(profile_).

BUG=
TBR=erikwright@chromium.org

Committed: https://crrev.com/0ea3fa90a36f091c943e286ade39b0252e5d34d7
Cr-Commit-Position: refs/heads/master@{#301636}

[Reilly Grant (use Gerrit), 2014-10-21 23:03:32]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2014-10-22 00:20:28]

Patchset #1 (id:20001) has been deleted

[Reilly Grant (use Gerrit), 2014-10-22 00:57:03]

Patchset #1 (id:40001) has been deleted

[Reilly Grant (use Gerrit), 2014-10-23 18:11:06]

Patchset #1 (id:60001) has been deleted

[Reilly Grant (use Gerrit), 2014-10-24 18:32:06]

Patchset #1 (id:80001) has been deleted

[Reilly Grant (use Gerrit), 2014-10-24 18:45:41]

reillyg@chromium.org changed reviewers:
+ kalman@chromium.org

[Reilly Grant (use Gerrit), 2014-10-24 20:05:55]

Kalman, please take a look. Most of this is boilerplate. The interesting stuff
is in ProcessManager and the new ProcessManagerFactory as well as the
ChromeProcessManagerDelegate as that was the hardest to get right.

[Reilly Grant (use Gerrit), 2014-10-24 20:22:00]

Patchset #1 (id:100001) has been deleted

[not at google - send to devlin, 2014-10-24 20:29:47]

Just the one real comment.

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:130: if
(!ExtensionSystem::Get(profile)->ready().is_signaled())
This does seem like more accurate code given the comment above it.

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
File extensions/browser/process_manager_factory.h (right):

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
extensions/browser/process_manager_factory.h:14: 
Typically these Factories are implementation details of the classes themselves,
and are hidden away in the .cc file, if I'm understanding this. I.e. the only
part of this class you need to expose anywhere is what gets called by
ProcessManager::Get.

[Reilly Grant (use Gerrit), 2014-10-24 20:37:23]

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:130: if
(!ExtensionSystem::Get(profile)->ready().is_signaled())
On 2014/10/24 20:29:47, kalman wrote:
> This does seem like more accurate code given the comment above it.

Well, now that the ProcessManager is an independant service ::Get will almost
never return NULL so the old check didn't make sense. ^_^

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
File extensions/browser/process_manager_factory.h (right):

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
extensions/browser/process_manager_factory.h:14: 
On 2014/10/24 20:29:47, kalman wrote:
> Typically these Factories are implementation details of the classes
themselves,
> and are hidden away in the .cc file, if I'm understanding this. I.e. the only
> part of this class you need to expose anywhere is what gets called by
> ProcessManager::Get.

The coding standard suggests one class per .cc file. There are also consumers
that directly access the factory directly, such as services that depend on it
and places where the "IfExists" version of Get is called. I put this method only
on the factory since it feels like more of a factory detail than something that
belongs in ProcessManager.

[not at google - send to devlin, 2014-10-24 20:52:03]

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
File extensions/browser/process_manager_factory.h (right):

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
extensions/browser/process_manager_factory.h:14: 
On 2014/10/24 20:37:23, reillyg wrote:
> On 2014/10/24 20:29:47, kalman wrote:
> > Typically these Factories are implementation details of the classes
> themselves,
> > and are hidden away in the .cc file, if I'm understanding this. I.e. the
only
> > part of this class you need to expose anywhere is what gets called by
> > ProcessManager::Get.
> 
> The coding standard suggests one class per .cc file. There are also consumers
> that directly access the factory directly, such as services that depend on it
> and places where the "IfExists" version of Get is called. I put this method
only
> on the factory since it feels like more of a factory detail than something
that
> belongs in ProcessManager.

Ah ok. Yeah I think you might need it in a separate file for declaring
dependencies anyway?

But I hardly see any normal code using the Factory versions, always accessing
through the ProcessManager::Get interface. You could add
ProcessManager::GetIfExists, but where do you need it? I've never seen it
before, so perhaps there is another way to achieve the same thing.

[Reilly Grant (use Gerrit), 2014-10-24 21:01:18]

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
File extensions/browser/process_manager_factory.h (right):

https://codereview.chromium.org/671763002/diff/120001/extensions/browser/proc...
extensions/browser/process_manager_factory.h:14: 
On 2014/10/24 20:52:03, kalman wrote:
> On 2014/10/24 20:37:23, reillyg wrote:
> > On 2014/10/24 20:29:47, kalman wrote:
> > > Typically these Factories are implementation details of the classes
> > themselves,
> > > and are hidden away in the .cc file, if I'm understanding this. I.e. the
> only
> > > part of this class you need to expose anywhere is what gets called by
> > > ProcessManager::Get.
> > 
> > The coding standard suggests one class per .cc file. There are also
consumers
> > that directly access the factory directly, such as services that depend on
it
> > and places where the "IfExists" version of Get is called. I put this method
> only
> > on the factory since it feels like more of a factory detail than something
> that
> > belongs in ProcessManager.
> 
> Ah ok. Yeah I think you might need it in a separate file for declaring
> dependencies anyway?

I don't *need* to but I'd rather keep it separate. Especially since there's the
whole IncognitoProcessManager thing that may get split out of process_manager.cc
and the logic for deciding which one to build could move into the factory.

> But I hardly see any normal code using the Factory versions, always accessing
> through the ProcessManager::Get interface. You could add
> ProcessManager::GetIfExists, but where do you need it? I've never seen it
> before, so perhaps there is another way to achieve the same thing.

ChromeProcessManagerDelegate needs GetIfExists it to be able to avoid creating a
new ProcessManager during profile destruction. I kept it in
ProcessManagerFactory because it feels more like asking the factory "did you
create one of these already" rather than something ProcessManager would know
about. I can add the wrapper if you think it would feel more consistent that
way.

[not at google - send to devlin, 2014-10-24 22:13:11]

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:145:
ProcessManagerFactory::GetForBrowserContextIfExists(profile);
Let's continue that discussion over here.

It seems like now that the ProcessManager is a KeyedService it can override the
Shutdown method and close the background hosts by itself? I don't really get why
the Delegate is doing this stuff.

[Reilly Grant (use Gerrit), 2014-10-24 22:17:40]

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):

https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/chrome_process_manager_delegate.cc:145:
ProcessManagerFactory::GetForBrowserContextIfExists(profile);
On 2014/10/24 22:13:10, kalman wrote:
> Let's continue that discussion over here.
> 
> It seems like now that the ProcessManager is a KeyedService it can override
the
> Shutdown method and close the background hosts by itself? I don't really get
why
> the Delegate is doing this stuff.

That's a good idea. Let me give it a try and see what breaks.

[Reilly Grant (use Gerrit), 2014-10-27 17:37:19]

On 2014/10/24 22:17:40, reillyg wrote:
>
https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
> File chrome/browser/extensions/chrome_process_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/671763002/diff/120001/chrome/browser/extensio...
> chrome/browser/extensions/chrome_process_manager_delegate.cc:145:
> ProcessManagerFactory::GetForBrowserContextIfExists(profile);
> On 2014/10/24 22:13:10, kalman wrote:
> > Let's continue that discussion over here.
> > 
> > It seems like now that the ProcessManager is a KeyedService it can override
> the
> > Shutdown method and close the background hosts by itself? I don't really get
> why
> > the Delegate is doing this stuff.
> 
> That's a good idea. Let me give it a try and see what breaks.

My understanding of the shutdown order described in
ProfileDestroyer::DestroyProfileWhenAppropriate is that KeyedService::Shutdown
is too late to be shutting down render hosts so the explicit notification from
the Profile that ChromeProcessManagerDelegate listens for is necessary.

[not at google - send to devlin, 2014-10-27 18:35:56]

lgtm as discussed let's go with this and simplify the awkward browser context
shutdown problem in a follow up.

[Reilly Grant (use Gerrit), 2014-10-27 18:51:12]

reillyg@chromium.org changed reviewers:
+ dgozman@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 18:52:46]

reillyg@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:03:32]

reillyg@chromium.org changed reviewers:
+ dconnelly@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:06:01]

reillyg@chromium.org changed reviewers:
+ mattm@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:07:46]

reillyg@chromium.org changed reviewers:
+ pkasting@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:10:26]

reillyg@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:13:21]

reillyg@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:15:39]

reillyg@chromium.org changed reviewers:
+ droger@chromium.org, nick@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 19:53:43]

Adding dgozman@ for chrome/browser/devtools, xiyuan@ for
chrome/browser/chromeos, dconnelly@ for chrome/browser/policy, mattm@ for
chrome/browser/net, pkasting@ for chrome/browser/autocomplete and
chrome/browser/ui, lazyboy@ for components/renderer_context_menu, dmazzoni@ for
chrome/browser/speech, nick@ for chrome/browser/task_manager and droger@ for
chrome/browser/translate.

[xiyuan, 2014-10-27 19:55:22]

c/b/chromeos/* LGTM

[lazyboy, 2014-10-27 20:30:09]

renderer_context_menu/* LGTM, but there's no OWNERS under that dir, you'd need a
components/* OWNER to review as well.

[Lei Zhang, 2014-10-27 20:49:44]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-10-27 20:49:45]

chrome/ outside of chrome/browser/extensions lgtm. (drive-by)

[Reilly Grant (use Gerrit), 2014-10-27 21:22:32]

reillyg@chromium.org changed reviewers:
+ jochen@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 21:22:34]

Adding jochen@ for components/render_context_menu since lazyboy@ isn't actually
an owner for it.

[Reilly Grant (use Gerrit), 2014-10-27 21:35:50]

reillyg@chromium.org changed reviewers:
+ erikwright@chromium.org
- jochen@chromium.org

[Reilly Grant (use Gerrit), 2014-10-27 21:36:22]

On 2014/10/27 21:22:34, reillyg wrote:
> Adding jochen@ for components/render_context_menu since lazyboy@ isn't
actually
> an owner for it.

jochen@ is OOO, trying erikwright@ instead.

[Peter Kasting, 2014-10-27 21:51:52]

LGTM

[mattm, 2014-10-28 03:01:47]

chrome/browser/net/ lgtm

[dgozman, 2014-10-28 05:16:47]

chrome/browser/devtools lgtm

[droger, 2014-10-28 07:52:08]

Translate lgtm

[dconnelly, 2014-10-28 09:59:56]

chrome/browser/policy lgtm

[Reilly Grant (use Gerrit), 2014-10-28 14:41:58]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-10-28 14:42:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/671763002/140001

[commit-bot: I haz the power, 2014-10-28 15:30:38]

Committed patchset #2 (id:140001)

[commit-bot: I haz the power, 2014-10-28 15:31:21]

Patchset 2 (id:??) landed as
https://crrev.com/0ea3fa90a36f091c943e286ade39b0252e5d34d7
Cr-Commit-Position: refs/heads/master@{#301636}