Unifying ChildProcessLauncher across platforms.

Unifying ChildProcessLauncher across platforms.

Refactored ChildProcessLauncher in an attempt to unify the API across
platforms and split the implementation in platform specific files,
replacing the existing ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is
used with the various posted tasks (ChildProcessLauncher is owned by the
client code and can be destroyed at any point) and contains most of the
platform specific code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non
trivial shared code between Posix implementations is in
child_process_launcher_posix.cc.


BUG=675294
Review-Url: https://codereview.chromium.org/2594203004
Cr-Commit-Position: refs/heads/master@{#444442}
Committed: https://chromium.googlesource.com/chromium/src/+/828cd7f49f285857a741f6f255193ca6bbb011cd

[Jay Civelli, 2017-01-05 23:53:04]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:53:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 00:12:35]

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

[commit-bot: I haz the power, 2017-01-06 00:12:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jay Civelli, 2017-01-10 01:11:10]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 01:12:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-01-10 01:14:10]

Description was changed from

==========
Make child process launcher great again.

BUG=675294
==========

to

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files.
In some cases, some minimal amount of code is duplicated in those platform
specific files but it keeps the code clearer and easier to follow.

BUG=675294
==========

[commit-bot: I haz the power, 2017-01-10 03:06:33]

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

[commit-bot: I haz the power, 2017-01-10 03:06:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jay Civelli, 2017-01-11 00:55:50]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[Jay Civelli, 2017-01-11 00:56:06]

jcivelli@chromium.org changed reviewers:
+ boliu@chromium.org, jam@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:56:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-01-11 00:58:55]

Description was changed from

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files.
In some cases, some minimal amount of code is duplicated in those platform
specific files but it keeps the code clearer and easier to follow.

BUG=675294
==========

to

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files (instead of the
existing ifdef's spaghetti code).
In some cases, some minimal amount of code is duplicated in those platform
specific files but it keeps the code clearer and easier to follow.

BUG=675294
==========

[Jay Civelli, 2017-01-11 01:00:20]

Description was changed from

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files (instead of the
existing ifdef's spaghetti code).
In some cases, some minimal amount of code is duplicated in those platform
specific files but it keeps the code clearer and easier to follow.

BUG=675294
==========

to

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files, replacing the existing
ifdef's spaghetti code.
In some cases, some minimal amount of code is duplicated in those platform
specific files, but it keeps the code clearer and easier to follow.

BUG=675294
==========

[commit-bot: I haz the power, 2017-01-11 04:42:18]

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

[commit-bot: I haz the power, 2017-01-11 04:42:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jay Civelli, 2017-01-11 17:57:18]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 17:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-01-11 17:58:40]

jcivelli@chromium.org changed reviewers:
+ rockot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:49:39]

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

[commit-bot: I haz the power, 2017-01-11 19:49:40]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-01-12 02:00:07]

I only traced through the android start up sequence, probably need to check
other platforms as well to see nothing has changed

Can you write a better CL description of what actually changed.

Some things can even have mappings from old code to new code.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.cc:137: auto region =
files_to_register->GetRegionAt(i);
I'm no c++11 expert, but I think this should be "const auto& region"

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.h (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:1: 
nit: empty line

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:2: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
cc file didn't have year updated, but header did?

If updating year, also remove (c)

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:77:
ChildProcessLauncher::Helper::Helper(
nit: group ChildProcessLauncher methods together and Helper methods together,
right now Helper methods are in the middle of ChildProcessLauncher methods

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:151: if (process.IsValid()) {
on android, old code didn't check this to record histogram

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:40: typedef
base::HandlesToInheritVector FileMappedForLaunch;
nit: prefer c++11 alias

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:82: class Helper : public
base::RefCountedThreadSafe<Helper> {
maybe add a comment that helper is the platform-specific implementation part of
launcher?

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:91: void BeforeLaunchOnClientThread();
group methods that are *actually* implemented by platform specific code
together?

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:104: bool ShouldForkAsZygote();
just curious.. and I didn't read all platforms, but can this be made constexpr
so it's a compile time thing?

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017 for new files

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:47:
base::Bind(&ChildProcessLauncher::Helper::PostLaunchOnLauncherThread,
this is an extra thread hop compared to previous code. comment in old code said
this could already be the UI thread, so did a shortcut

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:107: if (GetProcessType() !=
switches::kZygoteProcess) {
this can be removed from android, zygote is not in the CHECK above?

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:122: return false;
indent

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:148: new
ChildProcessHelperHolder(this)));
what's responsible for deleting holder?

Can ChildProcessStartedCallback take a reference of the helper instead of using
a "Holder" here?

[Jay Civelli, 2017-01-12 22:58:00]

Patchset #12 (id:220001) has been deleted

[Jay Civelli, 2017-01-12 23:04:43]

Description was changed from

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files, replacing the existing
ifdef's spaghetti code.
In some cases, some minimal amount of code is duplicated in those platform
specific files, but it keeps the code clearer and easier to follow.

BUG=675294
==========

to

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files, replacing the existing
ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is used
with the various posted tasks (ChildProcessLauncher is owned by the client code
and can be destroyed at any point) and contains most of the platform specific
code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non trivial
shared code between Posix implementations is in child_process_launcher_posix.cc.


BUG=675294
==========

[Jay Civelli, 2017-01-12 23:05:41]

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.cc:137: auto region =
files_to_register->GetRegionAt(i);
On 2017/01/12 02:00:07, boliu wrote:
> I'm no c++11 expert, but I think this should be "const auto& region"

Done.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.h (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:1: 
On 2017/01/12 02:00:07, boliu wrote:
> nit: empty line

Done.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:2: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
On 2017/01/12 02:00:07, boliu wrote:
> cc file didn't have year updated, but header did?
> 
> If updating year, also remove (c)

Keeping year as is, the file has been renamed with limited changes.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:77:
ChildProcessLauncher::Helper::Helper(
On 2017/01/12 02:00:07, boliu wrote:
> nit: group ChildProcessLauncher methods together and Helper methods together,
> right now Helper methods are in the middle of ChildProcessLauncher methods

Done (they were defined in the order they were declared in the .h file)

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:151: if (process.IsValid()) {
On 2017/01/12 02:00:07, boliu wrote:
> on android, old code didn't check this to record histogram

Yes, and that seemed wrong (the other platforms behavior seemed right). I don't
think we want to include the time it takes for a process to fail to start in the
metrics measuring how long it takes for a process to start.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:40: typedef
base::HandlesToInheritVector FileMappedForLaunch;
On 2017/01/12 02:00:07, boliu wrote:
> nit: prefer c++11 alias

Done.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:82: class Helper : public
base::RefCountedThreadSafe<Helper> {
On 2017/01/12 02:00:07, boliu wrote:
> maybe add a comment that helper is the platform-specific implementation part
of
> launcher?

Added a comment.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:91: void BeforeLaunchOnClientThread();
On 2017/01/12 02:00:07, boliu wrote:
> group methods that are *actually* implemented by platform specific code
> together?

I like to have them in the order they are invoked, makes understanding the flow
easier.
I have added comments to indicates which ones are platform specific.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher.h:104: bool ShouldForkAsZygote();
On 2017/01/12 02:00:07, boliu wrote:
> just curious.. and I didn't read all platforms, but can this be made constexpr
> so it's a compile time thing?

It depends on command line provided flags on Linux, so no.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/12 02:00:07, boliu wrote:
> 2017 for new files

Done.
This was written in 2016 though :-)

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:47:
base::Bind(&ChildProcessLauncher::Helper::PostLaunchOnLauncherThread,
On 2017/01/12 02:00:07, boliu wrote:
> this is an extra thread hop compared to previous code. comment in old code
said
> this could already be the UI thread, so did a shortcut

True, this seems to be called mostly on the UI thread (from what I saw).
Changed it so PostLaunchOnLauncherThread() has a param whether to call
PostLaunchOnClientThread. I don't like that it makes the API more complicated
and the flow less clear (sometimes PostLaunchOnLauncherThread can be called
after PostLaunchOnClientThread), but that extra hop may affect startup
performance.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:107: if (GetProcessType() !=
switches::kZygoteProcess) {
On 2017/01/12 02:00:07, boliu wrote:
> this can be removed from android, zygote is not in the CHECK above?

Good point, done.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:122: return false;
On 2017/01/12 02:00:07, boliu wrote:
> indent

Done.

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:148: new
ChildProcessHelperHolder(this)));
On 2017/01/12 02:00:07, boliu wrote:
> what's responsible for deleting holder?
> 
> Can ChildProcessStartedCallback take a reference of the helper instead of
using
> a "Holder" here?

Good point, I can take care of increasing the ref count myself (to guarantee the
helper is still around by the time the callback is called) and then don't need
the holder.

[Jay Civelli, 2017-01-12 23:10:28]

On 2017/01/12 02:00:07, boliu wrote:
> I only traced through the android start up sequence, probably need to check
> other platforms as well to see nothing has changed
I ran child_process_launcher.cc through the pre-processor to write this change
to check I kept the same behavior.
> Can you write a better CL description of what actually changed.
I added to it. The behavior should be the same ultimately, it's mostly
rearranging the code.
> 
> Some things can even have mappings from old code to new code.
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
> File content/browser/android/child_process_launcher_android_jni.cc (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
> content/browser/android/child_process_launcher_android_jni.cc:137: auto region
=
> files_to_register->GetRegionAt(i);
> I'm no c++11 expert, but I think this should be "const auto& region"
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
> File content/browser/android/child_process_launcher_android_jni.h (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
> content/browser/android/child_process_launcher_android_jni.h:1: 
> nit: empty line
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/androi...
> content/browser/android/child_process_launcher_android_jni.h:2: // Copyright
(c)
> 2016 The Chromium Authors. All rights reserved.
> cc file didn't have year updated, but header did?
> 
> If updating year, also remove (c)
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> File content/browser/child_process_launcher.cc (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.cc:77:
> ChildProcessLauncher::Helper::Helper(
> nit: group ChildProcessLauncher methods together and Helper methods together,
> right now Helper methods are in the middle of ChildProcessLauncher methods
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.cc:151: if (process.IsValid()) {
> on android, old code didn't check this to record histogram
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> File content/browser/child_process_launcher.h (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.h:40: typedef
> base::HandlesToInheritVector FileMappedForLaunch;
> nit: prefer c++11 alias
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.h:82: class Helper : public
> base::RefCountedThreadSafe<Helper> {
> maybe add a comment that helper is the platform-specific implementation part
of
> launcher?
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.h:91: void
BeforeLaunchOnClientThread();
> group methods that are *actually* implemented by platform specific code
> together?
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher.h:104: bool ShouldForkAsZygote();
> just curious.. and I didn't read all platforms, but can this be made constexpr
> so it's a compile time thing?
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> File content/browser/child_process_launcher_android.cc (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:1: // Copyright 2016 The
> Chromium Authors. All rights reserved.
> 2017 for new files
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:47:
> base::Bind(&ChildProcessLauncher::Helper::PostLaunchOnLauncherThread,
> this is an extra thread hop compared to previous code. comment in old code
said
> this could already be the UI thread, so did a shortcut
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:107: if (GetProcessType() !=
> switches::kZygoteProcess) {
> this can be removed from android, zygote is not in the CHECK above?
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:122: return false;
> indent
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:148: new
> ChildProcessHelperHolder(this)));
> what's responsible for deleting holder?
> 
> Can ChildProcessStartedCallback take a reference of the helper instead of
using
> a "Holder" here?

[boliu, 2017-01-13 01:33:50]

didn't have time to look at more code, but had one reply

also CL descriptions follow git convention, so one-line summary, then a blank
line, then paragraphs of description, and should be wrapped at 72 chars too I
believe

> I ran child_process_launcher.cc through the pre-processor to write this change
> to check I kept the same behavior.

Oh cool, so no need to trace through other platforms?

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:148: new
ChildProcessHelperHolder(this)));
On 2017/01/12 23:05:41, Jay Civelli wrote:
> On 2017/01/12 02:00:07, boliu wrote:
> > what's responsible for deleting holder?
> > 
> > Can ChildProcessStartedCallback take a reference of the helper instead of
> using
> > a "Holder" here?
> 
> Good point, I can take care of increasing the ref count myself (to guarantee
the
> helper is still around by the time the callback is called) and then don't need
> the holder.

This is not necessary, also not correct if the callback doesn't run. It should
just work if you change the function signature to take scoped_refptr:
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass...

[Jay Civelli, 2017-01-13 17:57:19]

Description was changed from

==========
Refactored ChildProcessLauncher in an attempt to unify the API across platforms
and split the implementation in platform specific files, replacing the existing
ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is used
with the various posted tasks (ChildProcessLauncher is owned by the client code
and can be destroyed at any point) and contains most of the platform specific
code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non trivial
shared code between Posix implementations is in child_process_launcher_posix.cc.


BUG=675294
==========

to

==========
Unifying ChildProcessLauncher across platforms.

Refactored ChildProcessLauncher in an attempt to unify the API across
platforms and split the implementation in platform specific files,
replacing the existing ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is
used with the various posted tasks (ChildProcessLauncher is owned by the
client code and can be destroyed at any point) and contains most of the
platform specific code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non
trivial shared code between Posix implementations is in
child_process_launcher_posix.cc.


BUG=675294
==========

[Jay Civelli, 2017-01-13 18:07:26]

On 2017/01/13 01:33:50, boliu wrote:
> didn't have time to look at more code, but had one reply
> 
> also CL descriptions follow git convention, so one-line summary, then a blank
> line, then paragraphs of description, and should be wrapped at 72 chars too I
> believe
Done.

> > I ran child_process_launcher.cc through the pre-processor to write this
change
> > to check I kept the same behavior.
> 
> Oh cool, so no need to trace through other platforms?
I'll go through them again to be thorough.

>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> File content/browser/child_process_launcher_android.cc (right):
> 
>
https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
> content/browser/child_process_launcher_android.cc:148: new
> ChildProcessHelperHolder(this)));
> On 2017/01/12 23:05:41, Jay Civelli wrote:
> > On 2017/01/12 02:00:07, boliu wrote:
> > > what's responsible for deleting holder?
> > > 
> > > Can ChildProcessStartedCallback take a reference of the helper instead of
> > using
> > > a "Holder" here?
> > 
> > Good point, I can take care of increasing the ref count myself (to guarantee
> the
> > helper is still around by the time the callback is called) and then don't
need
> > the holder.
> 
> This is not necessary, also not correct if the callback doesn't run. It should
> just work if you change the function signature to take scoped_refptr:
>
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass...

[Jay Civelli, 2017-01-13 18:08:01]

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/200001/content/browser/child_...
content/browser/child_process_launcher_android.cc:148: new
ChildProcessHelperHolder(this)));
On 2017/01/13 01:33:50, boliu wrote:
> On 2017/01/12 23:05:41, Jay Civelli wrote:
> > On 2017/01/12 02:00:07, boliu wrote:
> > > what's responsible for deleting holder?
> > > 
> > > Can ChildProcessStartedCallback take a reference of the helper instead of
> > using
> > > a "Holder" here?
> > 
> > Good point, I can take care of increasing the ref count myself (to guarantee
> the
> > helper is still around by the time the callback is called) and then don't
need
> > the holder.
> 
> This is not necessary, also not correct if the callback doesn't run. It should
> just work if you change the function signature to take scoped_refptr:
>
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass...

The reason we don't need to do this most of the time is because the callback
object is passed to a task runner when posted, and then owned by that task
runner, keeping the bound parameters alive.
In this case we are not posting to a task runner, we are passing it to Java as a
ptr. The way we keep things alive is by making a copy of the callback allocated
on the heap (see
https://cs.chromium.org/chromium/src/content/browser/android/child_process_la...
the code has moved to child_process_launcher_android_jni.cc in this CL but is
unchanged). The callback deletes itself when invoked (by Java), so it will be
leaked if the callback is never called already.
To make along story short, you are right I do not need to addref/release, since
the callback allocated on the heap keeps the bound parameters alive.

[jam, 2017-01-13 18:42:20]

first round of comments

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:40: using FileMappedForLaunch =
base::HandlesToInheritVector ;
nit: space at end

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:82: // The Helper class provides the
platform specific implementation of the child
nit: can we put this in the end of the public section? that way someone reading
this can just look at what they need as a consumer.

or even perhaps should this be declared in a separate
child_process_launcher_internal.h to make this separation clearer?

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:86: class Helper : public
base::RefCountedThreadSafe<Helper> {
hmm, implementation inheritance is something I've learnt to avoid. I'm curious
why you chose to go down this route vs keep the member variables on
ChildProcessLauncher, and have this strictly be an interface?

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:114: bool ShouldForkAsZygote();
why not add ifdefs here to make it clear these methods aren't called on win/mac?


i realize you're trying to reduce the ifdef soup, which is a good improvement.
but seems like most of that can come from the separate implementation files?

https://codereview.chromium.org/2594203004/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:201: // #include
"content/browser/android/child_process_launcher_android.h"
nit: remove

https://codereview.chromium.org/2594203004/diff/260001/content/public/browser...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/2594203004/diff/260001/content/public/browser...
content/public/browser/file_descriptor_info.h:26: #if !defined(OS_WIN)
not needed anymore?

[boliu, 2017-01-13 19:00:56]

all comments on older PS14, except that one about binding

https://codereview.chromium.org/2594203004/diff/240001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:15: // Contains the
methods either being called from or calling to
should we just fold this into the new child_process_launcher_android.h/cc files?
perhaps in a follow up though, this is helpful to show most things didn't change
in this file in this CL

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.cc:117: // 
DCHECK(CalledOnValidThread());
nit: empty space change

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:128: // |is_synchronous_launch| is set
to true if the starting of the process is
this comment is backwards

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:146: // process wasn't started
successfully.
|process| validity, or whether launch was successful doesn't affect whether this
is called though, right?

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:164: virtual ~Helper();
no subclasses, so doesn't need to be virtual or protected

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:250: static void Terminate(ZygoteHandle
zygote, base::Process process);
nit, same name as method on line 237, which is a bit confusing, give it a
different name, DoTerminate?

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:252: ZygoteHandle zygote, base::Process
process);
these two are also platform, should they move to helper class to be consistent?

or leave a comment

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_android.cc:41: base::Process(handle),
nullptr, launch_result, false);
/* post_launch_on_client_thread_called */ on false

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_linux.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_linux.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
all new files 2017?

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_win.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_win.cc:84: // TODO: deal with the result.
is this already addressed?

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_win.cc:85: return  process;
nit: extra space

https://codereview.chromium.org/2594203004/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:201: // #include
"content/browser/android/child_process_launcher_android.h"
remove

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
content/public/browser/browser_message_filter.cc:164:
ChildProcessLauncher::TerminateProcess(
there's a comment on TerminateProcess that process has to be started by
ChildProcessLauncher. that doesn't hold for single-process

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
content/public/browser/file_descriptor_info.h:26: #if !defined(OS_WIN)
maybe should just compile this out of windows altogether?

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher_android.cc:145:
base::Bind(&ChildProcessStartedCallback, Unretained(this)));
you have to remove Unretained and change ChildProcessStartedCallback signature
to take scoped_refptr

or you can use RetainedRef

or the brand new BindOnce to use move semantics with scoped_refptr

[Jay Civelli, 2017-01-15 08:09:07]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-15 08:09:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-15 09:36:46]

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

[commit-bot: I haz the power, 2017-01-15 09:36:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-01-15 20:11:52]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-15 20:12:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-15 20:41:11]

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

[commit-bot: I haz the power, 2017-01-15 20:41:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jay Civelli, 2017-01-16 06:58:23]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 06:58:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 07:37:28]

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

[commit-bot: I haz the power, 2017-01-16 07:37:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Jay Civelli, 2017-01-16 16:19:30]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 16:19:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 17:26:04]

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

[commit-bot: I haz the power, 2017-01-16 17:26:05]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-01-17 17:49:50]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[Jay Civelli, 2017-01-17 17:50:03]

https://codereview.chromium.org/2594203004/diff/240001/content/browser/androi...
File content/browser/android/child_process_launcher_android_jni.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/androi...
content/browser/android/child_process_launcher_android_jni.h:15: // Contains the
methods either being called from or calling to
On 2017/01/13 19:00:55, boliu wrote:
> should we just fold this into the new child_process_launcher_android.h/cc
files?
> perhaps in a follow up though, this is helpful to show most things didn't
change
> in this file in this CL

Since I moved the ChildProcessLauncherHelper to its own class I could rename
this one to child_process_launcher_android.cc
I must admit I kind of like having it split this way (JNI methods on one side,
content in another), but maybe you are right and it'd make more sense to have
them only in one file.
Will look into it as a follow up.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.cc:117: // 
DCHECK(CalledOnValidThread());
On 2017/01/13 19:00:55, boliu wrote:
> nit: empty space change

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:128: // |is_synchronous_launch| is set
to true if the starting of the process is
On 2017/01/13 19:00:55, boliu wrote:
> this comment is backwards

Fixed.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:146: // process wasn't started
successfully.
On 2017/01/13 19:00:55, boliu wrote:
> |process| validity, or whether launch was successful doesn't affect whether
this
> is called though, right?

Yes, attempted to clarify.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:164: virtual ~Helper();
On 2017/01/13 19:00:55, boliu wrote:
> no subclasses, so doesn't need to be virtual or protected

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:250: static void Terminate(ZygoteHandle
zygote, base::Process process);
On 2017/01/13 19:00:55, boliu wrote:
> nit, same name as method on line 237, which is a bit confusing, give it a
> different name, DoTerminate?

Renamed to the more explicit:
ForceNormalProcessTerminationAsync()

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher.h:252: ZygoteHandle zygote, base::Process
process);
On 2017/01/13 19:00:55, boliu wrote:
> these two are also platform, should they move to helper class to be
consistent?
> 
> or leave a comment

Moved to the helper class.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_android.cc:41: base::Process(handle),
nullptr, launch_result, false);
On 2017/01/13 19:00:55, boliu wrote:
> /* post_launch_on_client_thread_called */ on false

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_linux.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_linux.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/13 19:00:56, boliu wrote:
> all new files 2017?

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
File content/browser/child_process_launcher_win.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_win.cc:84: // TODO: deal with the result.
On 2017/01/13 19:00:56, boliu wrote:
> is this already addressed?

Yes, removed.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/child_...
content/browser/child_process_launcher_win.cc:85: return  process;
On 2017/01/13 19:00:56, boliu wrote:
> nit: extra space

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:201: // #include
"content/browser/android/child_process_launcher_android.h"
On 2017/01/13 19:00:56, boliu wrote:
> remove

Done.

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
content/public/browser/browser_message_filter.cc:164:
ChildProcessLauncher::TerminateProcess(
On 2017/01/13 19:00:56, boliu wrote:
> there's a comment on TerminateProcess that process has to be started by
> ChildProcessLauncher. that doesn't hold for single-process

I am not sure I am following you.
Do we support single process on Android? (in which case StopChildProcess would
not work if I understand correctly, and that's why I added the comment to
ChildProcessLauncher)
On other platforms, yes, we would not respect that restriction.
Clarified the comment in ChildProcessLauncher::TerminateProcess().

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
content/public/browser/file_descriptor_info.h:26: #if !defined(OS_WIN)
On 2017/01/13 19:00:56, boliu wrote:
> maybe should just compile this out of windows altogether?

Yep, this is not compiled on Windows.
Removed the ifdef (left over from some previous experimentation)

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:40: using FileMappedForLaunch =
base::HandlesToInheritVector ;
On 2017/01/13 18:42:20, jam wrote:
> nit: space at end

Done.

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:82: // The Helper class provides the
platform specific implementation of the child
On 2017/01/13 18:42:19, jam wrote:
> nit: can we put this in the end of the public section? that way someone
reading
> this can just look at what they need as a consumer.
> 
> or even perhaps should this be declared in a separate
> child_process_launcher_internal.h to make this separation clearer?

Moved to a new child_process_launcher_helper file.

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:86: class Helper : public
base::RefCountedThreadSafe<Helper> {
On 2017/01/13 18:42:19, jam wrote:
> hmm, implementation inheritance is something I've learnt to avoid. I'm curious
> why you chose to go down this route vs keep the member variables on
> ChildProcessLauncher, and have this strictly be an interface?

From our discussion, there is no inheritance, the methods are simply implemented
in platform specific files.

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher.h:114: bool ShouldForkAsZygote();
On 2017/01/13 18:42:19, jam wrote:
> why not add ifdefs here to make it clear these methods aren't called on
win/mac?
> 
> 
> i realize you're trying to reduce the ifdef soup, which is a good improvement.
> but seems like most of that can come from the separate implementation files?

I created a custom CLP Process struct that contains the base::Process and on
Windows, also the zygote. And I got rid of this zygote specific methods.

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
File content/browser/child_process_launcher_android.cc (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/child_...
content/browser/child_process_launcher_android.cc:145:
base::Bind(&ChildProcessStartedCallback, Unretained(this)));
On 2017/01/13 19:00:56, boliu wrote:
> you have to remove Unretained and change ChildProcessStartedCallback signature
> to take scoped_refptr
> 
> or you can use RetainedRef
> 
> or the brand new BindOnce to use move semantics with scoped_refptr

Used RetainedRef.

https://codereview.chromium.org/2594203004/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2594203004/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:201: // #include
"content/browser/android/child_process_launcher_android.h"
On 2017/01/13 18:42:20, jam wrote:
> nit: remove

Done.

https://codereview.chromium.org/2594203004/diff/260001/content/public/browser...
File content/public/browser/file_descriptor_info.h (right):

https://codereview.chromium.org/2594203004/diff/260001/content/public/browser...
content/public/browser/file_descriptor_info.h:26: #if !defined(OS_WIN)
On 2017/01/13 18:42:20, jam wrote:
> not needed anymore?

Oh, I thought I had already removed it.
Now really removed.

[commit-bot: I haz the power, 2017-01-17 17:50:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:49:24]

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

[commit-bot: I haz the power, 2017-01-17 18:49:25]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-01-17 23:28:41]

lgtm but I guess wait for other reviewers

I didn't trace through and compare code for other platforms like I did for
android last week.

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/2594203004/diff/240001/content/public/browser...
content/public/browser/browser_message_filter.cc:164:
ChildProcessLauncher::TerminateProcess(
On 2017/01/17 17:50:02, Jay Civelli wrote:
> On 2017/01/13 19:00:56, boliu wrote:
> > there's a comment on TerminateProcess that process has to be started by
> > ChildProcessLauncher. that doesn't hold for single-process
> 
> I am not sure I am following you.
> Do we support single process on Android?

Yes for webview.

> (in which case StopChildProcess would
> not work if I understand correctly, and that's why I added the comment to
> ChildProcessLauncher)
> On other platforms, yes, we would not respect that restriction.
> Clarified the comment in ChildProcessLauncher::TerminateProcess().

Good point. Looks like that's an existing bug then.
RenderProcessHostImpl::ShutdownForBadMessage has a run_renderer_in_process check
and then just suicides. But looks like there's no equivalent here. Filed
crbug.com/682023

[jam, 2017-01-18 17:58:39]

lgtm

[Jay Civelli, 2017-01-18 17:59:48]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-01-18 18:00:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 19:50:50]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1484762388482040,
"parent_rev": "8c0c9992b03751b32210ea1fa5ef2513da642cdb", "commit_rev":
"828cd7f49f285857a741f6f255193ca6bbb011cd"}

[commit-bot: I haz the power, 2017-01-18 19:51:18]

Description was changed from

==========
Unifying ChildProcessLauncher across platforms.

Refactored ChildProcessLauncher in an attempt to unify the API across
platforms and split the implementation in platform specific files,
replacing the existing ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is
used with the various posted tasks (ChildProcessLauncher is owned by the
client code and can be destroyed at any point) and contains most of the
platform specific code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non
trivial shared code between Posix implementations is in
child_process_launcher_posix.cc.


BUG=675294
==========

to

==========
Unifying ChildProcessLauncher across platforms.

Refactored ChildProcessLauncher in an attempt to unify the API across
platforms and split the implementation in platform specific files,
replacing the existing ifdef's spaghetti code.
A new ref counted Helper class is added to ChildProcessLauncher that is
used with the various posted tasks (ChildProcessLauncher is owned by the
client code and can be destroyed at any point) and contains most of the
platform specific code.
In some cases, some minimal amount of code is duplicated in the platform
specific files, but it keeps the code clearer and easier to follow. Non
trivial shared code between Posix implementations is in
child_process_launcher_posix.cc.


BUG=675294

Review-Url: https://codereview.chromium.org/2594203004
Cr-Commit-Position: refs/heads/master@{#444442}
Committed:
https://chromium.googlesource.com/chromium/src/+/828cd7f49f285857a741f6f25519...
==========

[commit-bot: I haz the power, 2017-01-18 19:51:20]

Committed patchset #17 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/828cd7f49f285857a741f6f25519...