Share field trial allocator on zygote-using Linuxes

Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
Committed: https://crrev.com/c4fe88004d0457cf00b1731c1e974c96a3cd649e
Cr-Commit-Position: refs/heads/master@{#435284}

[lawrencewu, 2016-11-23 15:45:01]

Description was changed from

==========
Share field trial allocator on Linux

BUG=663914
==========

to

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle to shared
memory,
which is a file descriptor (since shared memory is really just an unnamed file),
to
files_to_register. The fds in this mapping get remapped in the zygote process,
and
then all child processes fork off the zygote, so the fds are the same there.
When we spawn a new process, then, we add our fd as a switch on the command line
(--field-trial-handle).
Then, in the child process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a singleton that
maps
from the browser process's fd (key which we have) to the remapped fd on the
zygote
(actual fd containing shared memory). So we use that to look it up and then read
from
shared memory as normal.

BUG=663914
==========

[lawrencewu, 2016-11-23 15:45:01]

lawrencewu@chromium.org changed reviewers:
+ asvitkine@chromium.org

[lawrencewu, 2016-11-23 15:45:19]

Description was changed from

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle to shared
memory,
which is a file descriptor (since shared memory is really just an unnamed file),
to
files_to_register. The fds in this mapping get remapped in the zygote process,
and
then all child processes fork off the zygote, so the fds are the same there.
When we spawn a new process, then, we add our fd as a switch on the command line
(--field-trial-handle).
Then, in the child process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a singleton that
maps
from the browser process's fd (key which we have) to the remapped fd on the
zygote
(actual fd containing shared memory). So we use that to look it up and then read
from
shared memory as normal.

BUG=663914
==========

to

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle to shared
memory,
which is a file descriptor (since shared memory is really just an unnamed file),
to
files_to_register. The fds in this mapping get remapped in the zygote process,
and
then all child processes fork off the zygote, so the fds are the same there.
When we
spawn a new process, then, we add our fd as a switch on the command line
(--field-trial-handle).
Then, in the child process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a singleton that
maps
from the browser process's fd (key which we have) to the remapped fd on the
zygote
(actual fd containing shared memory). So we use that to look it up and then read
from
shared memory as normal.

BUG=663914
==========

[lawrencewu, 2016-11-23 15:46:51]

Description was changed from

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle to shared
memory,
which is a file descriptor (since shared memory is really just an unnamed file),
to
files_to_register. The fds in this mapping get remapped in the zygote process,
and
then all child processes fork off the zygote, so the fds are the same there.
When we
spawn a new process, then, we add our fd as a switch on the command line
(--field-trial-handle).
Then, in the child process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a singleton that
maps
from the browser process's fd (key which we have) to the remapped fd on the
zygote
(actual fd containing shared memory). So we use that to look it up and then read
from
shared memory as normal.

BUG=663914
==========

to

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
normal.

BUG=663914
==========

[lawrencewu, 2016-11-23 15:47:36]

Description was changed from

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
normal.

BUG=663914
==========

to

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

[lawrencewu, 2016-11-23 16:50:28]

Initial CL for sharing the field trial allocator on Linux. I will probably have
to add more ifdefs for Mac and Android since they don't use the zygote (not sure
about Android) -- working on that now.

[Alexei Svitkine (slow), 2016-11-23 17:49:55]

Initial pass.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:240: #elif defined(OS_POSIX) && !defined(OS_NACL)
Instead of these #elif chains that are confusing - just use separate ifdefs and
space them out (i.e. empty line between functions.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:246: return fd;
Nit: Merge with line above.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:517: // we can add it to the mapping there.
You check the return value of this for -1 but don't document that. Please do.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:518: static int FieldTrialHandle();
Functions should use a verb. GetFieldTrialHandle()?

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:565: static bool
CreateTrialsFromWindowsSwitch(std::string cli_switch);
Don't pass strings by value.

const std::string&

Also, avoid abbreviations/acronyms - so command_line_switch - or handle_switch.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:571: static bool
CreateTrialsFromPosixSwitch(std::string cli_switch);
Do we need two separate functions? Why not keeping it the same one and have the
details be in the implementation?

[lawrencewu, 2016-11-23 18:59:44]

Address comments.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:240: #elif defined(OS_POSIX) && !defined(OS_NACL)
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> Instead of these #elif chains that are confusing - just use separate ifdefs
and
> space them out (i.e. empty line between functions.

Done.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.cc:246: return fd;
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> Nit: Merge with line above.

Done.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:517: // we can add it to the mapping there.
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> You check the return value of this for -1 but don't document that. Please do.

Done.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:518: static int FieldTrialHandle();
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> Functions should use a verb. GetFieldTrialHandle()?

Done.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:565: static bool
CreateTrialsFromWindowsSwitch(std::string cli_switch);
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> Don't pass strings by value.
> 
> const std::string&
> 
> Also, avoid abbreviations/acronyms - so command_line_switch - or
handle_switch.

Done.

https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
base/metrics/field_trial.h:571: static bool
CreateTrialsFromPosixSwitch(std::string cli_switch);
On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> Do we need two separate functions? Why not keeping it the same one and have
the
> details be in the implementation?

Yeah, that works better. Done.

[lawrencewu, 2016-11-23 19:00:31]

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

[commit-bot: I haz the power, 2016-11-23 19:01:13]

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

[lawrencewu, 2016-11-23 19:19:31]

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

[commit-bot: I haz the power, 2016-11-23 19:20:19]

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

[lawrencewu, 2016-11-23 19:46:39]

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

[commit-bot: I haz the power, 2016-11-23 19:47:08]

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

[lawrencewu, 2016-11-23 20:14:38]

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

[commit-bot: I haz the power, 2016-11-23 20:15:06]

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

[lawrencewu, 2016-11-23 20:25:23]

Description was changed from

==========
Share field trial allocator on Linux

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

to

==========
Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

[commit-bot: I haz the power, 2016-11-23 20:57:37]

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

[commit-bot: I haz the power, 2016-11-23 20:57:38]

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...)

[lawrencewu, 2016-11-23 21:54:54]

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

[commit-bot: I haz the power, 2016-11-23 21:55:45]

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, 2016-11-23 22:41:46]

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

[commit-bot: I haz the power, 2016-11-23 22:41:47]

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

[lawrencewu, 2016-11-24 15:54:34]

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

[commit-bot: I haz the power, 2016-11-24 15:54:50]

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

[lawrencewu, 2016-11-24 16:33:28]

Verified that this falls back using the command line on Mac as intended.

On 2016/11/23 18:59:44, lawrencewu wrote:
> Address comments.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> File base/metrics/field_trial.cc (right):
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.cc:240: #elif defined(OS_POSIX) && !defined(OS_NACL)
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > Instead of these #elif chains that are confusing - just use separate ifdefs
> and
> > space them out (i.e. empty line between functions.
> 
> Done.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.cc:246: return fd;
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > Nit: Merge with line above.
> 
> Done.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_trial.h
> File base/metrics/field_trial.h (right):
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.h:517: // we can add it to the mapping there.
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > You check the return value of this for -1 but don't document that. Please
do.
> 
> Done.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.h:518: static int FieldTrialHandle();
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > Functions should use a verb. GetFieldTrialHandle()?
> 
> Done.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.h:565: static bool
> CreateTrialsFromWindowsSwitch(std::string cli_switch);
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > Don't pass strings by value.
> > 
> > const std::string&
> > 
> > Also, avoid abbreviations/acronyms - so command_line_switch - or
> handle_switch.
> 
> Done.
> 
>
https://codereview.chromium.org/2530573002/diff/40001/base/metrics/field_tria...
> base/metrics/field_trial.h:571: static bool
> CreateTrialsFromPosixSwitch(std::string cli_switch);
> On 2016/11/23 17:49:55, Alexei Svitkine (slow) wrote:
> > Do we need two separate functions? Why not keeping it the same one and have
> the
> > details be in the implementation?
> 
> Yeah, that works better. Done.

[commit-bot: I haz the power, 2016-11-24 16:41:35]

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

[commit-bot: I haz the power, 2016-11-24 16:41:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-24 16:55:57]

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

[commit-bot: I haz the power, 2016-11-24 16:56:19]

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, 2016-11-24 17:34:04]

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

[commit-bot: I haz the power, 2016-11-24 17:34:05]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-11-24 17:44:35]

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.cc:8: #endif
Not At the top of the file - put it after all the includes but before the
global_descriptors.h one.

Also, add a comment.

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.h:518: // |files_to_register| in
child_process_launcher.cc. This gets the handle so
I don't think this should mention child_process_launcher.cc - which is in a
different layer.

I suggest just describing this more generally - i.e. the file descriptor that
should be shared with the child process.

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.h:644: int readonly_allocator_handle_ = -1;
Hmm, how about having this be the same var on Win and Posix and use PlatformFile
as the type - which is typedef'd to int on POSIX and HANDLE on Windows.

Them, you can use kInvalidPlatformFile for the default value and document that
as the return value of the other function.

[lawrencewu, 2016-11-24 18:15:58]

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

[lawrencewu, 2016-11-24 18:16:13]

Address comments.

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.cc:8: #endif
On 2016/11/24 17:44:34, Alexei Svitkine (slow) wrote:
> Not At the top of the file - put it after all the includes but before the
> global_descriptors.h one.
> 
> Also, add a comment.

Done.

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.h:518: // |files_to_register| in
child_process_launcher.cc. This gets the handle so
On 2016/11/24 17:44:34, Alexei Svitkine (slow) wrote:
> I don't think this should mention child_process_launcher.cc - which is in a
> different layer.
> 
> I suggest just describing this more generally - i.e. the file descriptor that
> should be shared with the child process.

Done.

https://codereview.chromium.org/2530573002/diff/220001/base/metrics/field_tri...
base/metrics/field_trial.h:644: int readonly_allocator_handle_ = -1;
On 2016/11/24 17:44:34, Alexei Svitkine (slow) wrote:
> Hmm, how about having this be the same var on Win and Posix and use
PlatformFile
> as the type - which is typedef'd to int on POSIX and HANDLE on Windows.
> 
> Them, you can use kInvalidPlatformFile for the default value and document that
> as the return value of the other function.

That's much cleaner, done.

[commit-bot: I haz the power, 2016-11-24 18:16:22]

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

[Alexei Svitkine (slow), 2016-11-24 18:23:19]

https://codereview.chromium.org/2530573002/diff/260001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/260001/base/metrics/field_tri...
base/metrics/field_trial.cc:844: if (kUseSharedMemoryForFieldTrials) {
This block seems to be almost identical to the one above.

Can we make it identical and not have two separate blocks? e.g. we can keep the
code same as for Windows with the cast - and then the codepath would be the
same?

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

https://codereview.chromium.org/2530573002/diff/260001/content/browser/child_...
content/browser/child_process_launcher.cc:168: if (field_trial_handle != -1)
Check for the constant.

[lawrencewu, 2016-11-24 18:44:50]

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

[commit-bot: I haz the power, 2016-11-24 18:45:16]

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

[lawrencewu, 2016-11-24 18:48:59]

Address comments.

https://codereview.chromium.org/2530573002/diff/260001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/260001/base/metrics/field_tri...
base/metrics/field_trial.cc:844: if (kUseSharedMemoryForFieldTrials) {
On 2016/11/24 18:23:19, Alexei Svitkine (slow) wrote:
> This block seems to be almost identical to the one above.
> 
> Can we make it identical and not have two separate blocks? e.g. we can keep
the
> code same as for Windows with the cast - and then the codepath would be the
> same?

Made nearly identical, still have to do some windows-specific casting.

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

https://codereview.chromium.org/2530573002/diff/260001/content/browser/child_...
content/browser/child_process_launcher.cc:168: if (field_trial_handle != -1)
On 2016/11/24 18:23:19, Alexei Svitkine (slow) wrote:
> Check for the constant.

Done.

[Alexei Svitkine (slow), 2016-11-24 19:04:39]

lgtm

https://codereview.chromium.org/2530573002/diff/320001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/320001/base/metrics/field_tri...
base/metrics/field_trial.cc:799: if (global_->readonly_allocator_handle_ !=
kInvalidPlatformFile)
Nit: Seems this if is not needed - since if it's kInvalidPlatformFile we can
just return it anyway.

Maybe add a comment instead of the if?

[lawrencewu, 2016-11-24 19:14:57]

address nit

https://codereview.chromium.org/2530573002/diff/320001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/320001/base/metrics/field_tri...
base/metrics/field_trial.cc:799: if (global_->readonly_allocator_handle_ !=
kInvalidPlatformFile)
On 2016/11/24 19:04:39, Alexei Svitkine (slow) wrote:
> Nit: Seems this if is not needed - since if it's kInvalidPlatformFile we can
> just return it anyway.
> 
> Maybe add a comment instead of the if?

Done.

[lawrencewu, 2016-11-24 19:17:48]

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

[lawrencewu, 2016-11-24 19:17:49]

@jochen: mind looking at the changes to child_process_launcher.cc? Thanks in
advance.

[lawrencewu, 2016-11-24 22:12:18]

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

[commit-bot: I haz the power, 2016-11-24 22:12:31]

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, 2016-11-24 22:43:55]

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

[commit-bot: I haz the power, 2016-11-24 22:43:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-24 23:28:29]

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

[commit-bot: I haz the power, 2016-11-24 23:28:38]

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, 2016-11-24 23:40:05]

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

[commit-bot: I haz the power, 2016-11-24 23:40:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-25 01:10:55]

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

[commit-bot: I haz the power, 2016-11-25 01:11: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, 2016-11-25 01:45:34]

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

[commit-bot: I haz the power, 2016-11-25 01:45:35]

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...)

[lawrencewu, 2016-11-25 04:31:34]

@jochen: Hmm, maybe you should hold off, actually. I'll take a look at those red
bots.

On 2016/11/24 19:17:49, lawrencewu wrote:
> @jochen: mind looking at the changes to child_process_launcher.cc? Thanks in
> advance.

[lawrencewu, 2016-11-25 16:06:30]

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

[commit-bot: I haz the power, 2016-11-25 16:06:42]

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, 2016-11-25 17:11:19]

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

[commit-bot: I haz the power, 2016-11-25 17:11:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-28 16:10:57]

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

[commit-bot: I haz the power, 2016-11-28 16:11:16]

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, 2016-11-28 16:18:19]

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

[commit-bot: I haz the power, 2016-11-28 16:18:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[lawrencewu, 2016-11-28 16:27:38]

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

[commit-bot: I haz the power, 2016-11-28 16:28:01]

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

[lawrencewu, 2016-11-28 16:28:57]

@asvitkine, @jochen: mind taking another look?

[commit-bot: I haz the power, 2016-11-28 16:30:12]

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

[commit-bot: I haz the power, 2016-11-28 16:30:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-28 16:36:26]

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

[commit-bot: I haz the power, 2016-11-28 16:36:57]

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

[Alexei Svitkine (slow), 2016-11-28 16:56:13]

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.cc:842:
global_->field_trial_allocator_->UpdateTrackingHistograms();
We should also log this histogram on Linux.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.cc:847: AddForceFieldTrialsFlag(cmd_line);
This results in force field trials flag always being added on linux and thus
your new code path never being taken - since the flag is checked first in
CreateTrialsFromCommandLine().

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:84: #define POSIX_WITH_ZYGOTE 1
I don't think we should define this in the header file.

Otherwise, this define bleeds into all files including this header, which we
don't want. (If we wanted such a thing to exist, it should be in a more
appropriate place in base.)

Otherwise, suggest spelling things out for the one time it's used in the .h and
then keeping the define in the .cc.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:516: const int field_trial_handle);
Nit: No reason to mark primitive params (int) as const.

Also, what is "field_trial_handle"? It's not the actual handle - it looks like
it's the id in the posix global descriptors map. If so, name it as such and also
document it in the comment.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:584: static bool CreateTrialsFromFdKey(const int
fd_key);
Nit: No const

https://codereview.chromium.org/2530573002/diff/460001/components/variations/...
File components/variations/child_process_field_trial_syncer_unittest.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/components/variations/...
components/variations/child_process_field_trial_syncer_unittest.cc:58:
*base::CommandLine::ForCurrentProcess(), "field_trial_handle_switch", -1);
You don't document anywhere why it's OK to pass a -1 for the last param.

https://codereview.chromium.org/2530573002/diff/460001/content/public/common/...
File content/public/common/content_descriptors.h (right):

https://codereview.chromium.org/2530573002/diff/460001/content/public/common/...
content/public/common/content_descriptors.h:17: kFieldTrialHandle,
This name is very confusing since it's the same name as the command line flag on
Windows.

Suggest renaming - how about kFieldTrialDescriptor since this is posix code?

[commit-bot: I haz the power, 2016-11-28 17:14:39]

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

[commit-bot: I haz the power, 2016-11-28 17:14:40]

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

[lawrencewu, 2016-11-28 18:25:51]

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

[commit-bot: I haz the power, 2016-11-28 18:26:35]

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, 2016-11-28 19:35:52]

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

[commit-bot: I haz the power, 2016-11-28 19:35:54]

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

[lawrencewu, 2016-11-28 20:22:46]

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

[commit-bot: I haz the power, 2016-11-28 20:24:00]

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

[lawrencewu, 2016-11-28 20:27:20]

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

[commit-bot: I haz the power, 2016-11-28 20:28:06]

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, 2016-11-28 22:18:47]

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

[commit-bot: I haz the power, 2016-11-28 22:18:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[lawrencewu, 2016-11-28 23:01:42]

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

[commit-bot: I haz the power, 2016-11-28 23:03:32]

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, 2016-11-29 02:06:49]

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

[commit-bot: I haz the power, 2016-11-29 02:06:50]

Dry run: This issue passed the CQ dry run.

[lawrencewu, 2016-11-29 14:38:08]

Address comments and create a method CreateTrialsFromDescriptor() that we
attempt to use on Linux (and fallback to CreateTrialsFromCommandLine() if the
descriptor id doesn't exist).

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.cc:842:
global_->field_trial_allocator_->UpdateTrackingHistograms();
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> We should also log this histogram on Linux.

Done.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.cc:847: AddForceFieldTrialsFlag(cmd_line);
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> This results in force field trials flag always being added on linux and thus
> your new code path never being taken - since the flag is checked first in
> CreateTrialsFromCommandLine().

Fixed.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:84: #define POSIX_WITH_ZYGOTE 1
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> I don't think we should define this in the header file.
> 
> Otherwise, this define bleeds into all files including this header, which we
> don't want. (If we wanted such a thing to exist, it should be in a more
> appropriate place in base.)
> 
> Otherwise, suggest spelling things out for the one time it's used in the .h
and
> then keeping the define in the .cc.

Done.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:516: const int field_trial_handle);
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> Nit: No reason to mark primitive params (int) as const.
> 
> Also, what is "field_trial_handle"? It's not the actual handle - it looks like
> it's the id in the posix global descriptors map. If so, name it as such and
also
> document it in the comment.

Done.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial.h:584: static bool CreateTrialsFromFdKey(const int
fd_key);
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> Nit: No const

Done.

https://codereview.chromium.org/2530573002/diff/460001/components/variations/...
File components/variations/child_process_field_trial_syncer_unittest.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/components/variations/...
components/variations/child_process_field_trial_syncer_unittest.cc:58:
*base::CommandLine::ForCurrentProcess(), "field_trial_handle_switch", -1);
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> You don't document anywhere why it's OK to pass a -1 for the last param.

Changed things around so we don't need to pass in the last param anymore.

https://codereview.chromium.org/2530573002/diff/460001/content/public/common/...
File content/public/common/content_descriptors.h (right):

https://codereview.chromium.org/2530573002/diff/460001/content/public/common/...
content/public/common/content_descriptors.h:17: kFieldTrialHandle,
On 2016/11/28 16:56:13, Alexei Svitkine (slow) wrote:
> This name is very confusing since it's the same name as the command line flag
on
> Windows.
> 
> Suggest renaming - how about kFieldTrialDescriptor since this is posix code?

Done.

[jochen (gone - plz use gerrit), 2016-11-29 15:13:39]

https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
e.g. https://cs.chromium.org/chromium/src/content/public/common/zygote_handle.h
has a different condition. why OS_NACL?

[lawrencewu, 2016-11-29 15:24:44]

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

[lawrencewu, 2016-11-29 15:24:51]

https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
On 2016/11/29 15:13:39, jochen wrote:
> e.g.
https://cs.chromium.org/chromium/src/content/public/common/zygote_handle.h
> has a different condition. why OS_NACL?

I wasn't sure if NaCl was POSIX-compliant and if it was, whether it used the
zygote. I wanted to be sure that we wouldn't go down this codepath if it was.
After your comment, I did some research and it looks like it isn't, so I changed
the condition to reflect the one in zygote_handle.h.

[commit-bot: I haz the power, 2016-11-29 15:24:52]

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, 2016-11-29 15:39:08]

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

[commit-bot: I haz the power, 2016-11-29 15:39:09]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[lawrencewu, 2016-11-29 15:45:04]

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

[commit-bot: I haz the power, 2016-11-29 15:45:19]

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

[lawrencewu, 2016-11-29 15:52:35]

On 2016/11/29 15:24:51, lawrencewu wrote:
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
> On 2016/11/29 15:13:39, jochen wrote:
> > e.g.
> https://cs.chromium.org/chromium/src/content/public/common/zygote_handle.h
> > has a different condition. why OS_NACL?
> 
> I wasn't sure if NaCl was POSIX-compliant and if it was, whether it used the
> zygote. I wanted to be sure that we wouldn't go down this codepath if it was.
> After your comment, I did some research and it looks like it isn't, so I
changed
> the condition to reflect the one in zygote_handle.h.

Actually looks like OS_NACL ⊂ OS_POSIX, but since it doesn't appear in the ifdef
you linked to I assume
it uses the zygote as well, so it should still be fine.

[lawrencewu, 2016-11-29 15:52:36]

On 2016/11/29 15:24:51, lawrencewu wrote:
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
> On 2016/11/29 15:13:39, jochen wrote:
> > e.g.
> https://cs.chromium.org/chromium/src/content/public/common/zygote_handle.h
> > has a different condition. why OS_NACL?
> 
> I wasn't sure if NaCl was POSIX-compliant and if it was, whether it used the
> zygote. I wanted to be sure that we wouldn't go down this codepath if it was.
> After your comment, I did some research and it looks like it isn't, so I
changed
> the condition to reflect the one in zygote_handle.h.

Actually looks like OS_NACL ⊂ OS_POSIX, but since it doesn't appear in the ifdef
you linked to I assume
it uses the zygote as well, so it should still be fine.

[commit-bot: I haz the power, 2016-11-29 16:03:44]

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

[commit-bot: I haz the power, 2016-11-29 16:03:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-11-29 16:14:32]

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

[commit-bot: I haz the power, 2016-11-29 16:14:49]

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

[lawrencewu, 2016-11-29 16:15:47]

On 2016/11/29 15:52:36, lawrencewu wrote:
> On 2016/11/29 15:24:51, lawrencewu wrote:
> >
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> > File content/app/content_main_runner.cc (right):
> > 
> >
>
https://codereview.chromium.org/2530573002/diff/560001/content/app/content_ma...
> > content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
> > On 2016/11/29 15:13:39, jochen wrote:
> > > e.g.
> > https://cs.chromium.org/chromium/src/content/public/common/zygote_handle.h
> > > has a different condition. why OS_NACL?
> > 
> > I wasn't sure if NaCl was POSIX-compliant and if it was, whether it used the
> > zygote. I wanted to be sure that we wouldn't go down this codepath if it
was.
> > After your comment, I did some research and it looks like it isn't, so I
> changed
> > the condition to reflect the one in zygote_handle.h.
> 
> Actually looks like OS_NACL ⊂ OS_POSIX, but since it doesn't appear in the
ifdef
> you linked to I assume
> it uses the zygote as well, so it should still be fine.

OK, I spoke to asvitkine@ and he told me that we actually don't want this code
to support NaCl, since
it doesn't make sense to use this API from there. Given that, I reverted it back
to the original condition.

[commit-bot: I haz the power, 2016-11-29 17:18:44]

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

[commit-bot: I haz the power, 2016-11-29 17:18:45]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-11-29 17:38:27]

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1151: }
Suggest keeping this test and just make it Windows only.

https://codereview.chromium.org/2530573002/diff/620001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/620001/base/metrics/field_tri...
base/metrics/field_trial.cc:849: #if defined(OS_WIN)
Add a comment above this to mention why it's win only and how the handle is
passed on posix.

https://codereview.chromium.org/2530573002/diff/620001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/620001/content/app/content_ma...
content/app/content_main_runner.cc:156: }
I actually prefer you previous version which keeps the fall back logic within
the field trial code rather than leaking it here.

I think it's fine to add the extra param and you can just document on which
platforms each is used.

[lawrencewu, 2016-11-29 18:14:52]

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

[commit-bot: I haz the power, 2016-11-29 18:15:18]

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

[lawrencewu, 2016-11-29 18:33:10]

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

[commit-bot: I haz the power, 2016-11-29 18:33:41]

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

[lawrencewu, 2016-11-29 18:53:28]

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

[commit-bot: I haz the power, 2016-11-29 18:53:50]

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

[lawrencewu, 2016-11-29 19:11:01]

Revert to putting logic back in field_trial.cc and falling back from there.

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/2530573002/diff/460001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1151: }
On 2016/11/29 17:38:27, Alexei Svitkine (slow) wrote:
> Suggest keeping this test and just make it Windows only.

Done.

https://codereview.chromium.org/2530573002/diff/620001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/620001/base/metrics/field_tri...
base/metrics/field_trial.cc:849: #if defined(OS_WIN)
On 2016/11/29 17:38:27, Alexei Svitkine (slow) wrote:
> Add a comment above this to mention why it's win only and how the handle is
> passed on posix.

Done.

[lawrencewu, 2016-11-29 19:11:20]

https://codereview.chromium.org/2530573002/diff/620001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/620001/content/app/content_ma...
content/app/content_main_runner.cc:156: }
On 2016/11/29 17:38:27, Alexei Svitkine (slow) wrote:
> I actually prefer you previous version which keeps the fall back logic within
> the field trial code rather than leaking it here.
> 
> I think it's fine to add the extra param and you can just document on which
> platforms each is used.

Done.

[Alexei Svitkine (slow), 2016-11-29 19:32:47]

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:773: bool result =
CreateTrialsFromDescriptor(field_trial_descriptor_id);
Nit: Just inline this into the if.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:794: if (field_trial_descriptor_id == -1) {
Nit: No {}

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:803:
global_->create_trials_from_command_line_called_ = true;
This is not needed anymore since you're calling it from
CreateTrialsFromCommandLine.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:829: if (!global_)
Does this actually get hit?

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:862: // We need to pass a named anonymous handle to
shared memory over the command
Nit: Indent this comment.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:877:
global_->field_trial_allocator_->UpdateTrackingHistograms();
Nit: Move this line above to line 861.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.h:578: static bool CreateTrialsFromDescriptor(int
fd_key);
Nit: You use fd_key here but field_trial_descriptor_id elsewhere. Make
consistent (both in header and .cc).

I think fd_key is fine as a name since it's short and it still accurately
describes what it's doing.

https://codereview.chromium.org/2530573002/diff/720001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/content/app/content_ma...
content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
I think this can be simplified - this ifdef needs only to check if
kFieldTrialDescriptor has been included - which I think can just be OS_POSIX &&
! OS_MACOSX which matches the include.

https://codereview.chromium.org/2530573002/diff/720001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/content/public/test/re...
content/public/test/render_view_test.cc:275: // We don't use the descriptor here
anyways so it's ok to pass 1.
-1

[lawrencewu, 2016-11-29 19:51:51]

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

[commit-bot: I haz the power, 2016-11-29 19:52:17]

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

[lawrencewu, 2016-11-29 19:52:44]

Address comments.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:773: bool result =
CreateTrialsFromDescriptor(field_trial_descriptor_id);
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> Nit: Just inline this into the if.

Done.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:794: if (field_trial_descriptor_id == -1) {
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:803:
global_->create_trials_from_command_line_called_ = true;
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> This is not needed anymore since you're calling it from
> CreateTrialsFromCommandLine.

Removed.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:829: if (!global_)
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> Does this actually get hit?

I added a log statement here and it doesn't get hit -- but I don't see the harm
in keeping this check.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:862: // We need to pass a named anonymous handle to
shared memory over the command
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> Nit: Indent this comment.

Done.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:877:
global_->field_trial_allocator_->UpdateTrackingHistograms();
On 2016/11/29 19:32:46, Alexei Svitkine (slow) wrote:
> Nit: Move this line above to line 861.

Done.

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.h:578: static bool CreateTrialsFromDescriptor(int
fd_key);
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> Nit: You use fd_key here but field_trial_descriptor_id elsewhere. Make
> consistent (both in header and .cc).
> 
> I think fd_key is fine as a name since it's short and it still accurately
> describes what it's doing.

Done.

https://codereview.chromium.org/2530573002/diff/720001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/content/app/content_ma...
content/app/content_main_runner.cc:148: !defined(OS_ANDROID)
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> I think this can be simplified - this ifdef needs only to check if
> kFieldTrialDescriptor has been included - which I think can just be OS_POSIX
&&
> ! OS_MACOSX which matches the include.

I think this can even just be OS_POSIX.

https://codereview.chromium.org/2530573002/diff/720001/content/public/test/re...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/content/public/test/re...
content/public/test/render_view_test.cc:275: // We don't use the descriptor here
anyways so it's ok to pass 1.
On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> -1

Fixed.

[Alexei Svitkine (slow), 2016-11-29 20:19:28]

lgtm

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:829: if (!global_)
On 2016/11/29 19:52:44, lawrencewu wrote:
> On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> > Does this actually get hit?
> 
> I added a log statement here and it doesn't get hit -- but I don't see the
harm
> in keeping this check.

I think it would be better to remove this if it's not necessary.

https://codereview.chromium.org/2530573002/diff/740001/base/metrics/field_tri...
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/2530573002/diff/740001/base/metrics/field_tri...
base/metrics/field_trial.h:501: // get the trials from opening |fd_key| if using
shared
Nit: Update wrapping throughout this comment.

[lawrencewu, 2016-11-29 20:29:18]

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

[commit-bot: I haz the power, 2016-11-29 20:29:38]

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

[lawrencewu, 2016-11-29 20:32:13]

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

[lawrencewu, 2016-11-29 20:32:55]

Address nits and had to move a comment down since git cl format was forcing it
to have no indentation when it should have.

@jochen: mind taking another look?

[commit-bot: I haz the power, 2016-11-29 20:33:12]

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, 2016-11-29 21:19:12]

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

[commit-bot: I haz the power, 2016-11-29 21:19:14]

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

[lawrencewu, 2016-11-29 21:39:54]

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

[lawrencewu, 2016-11-29 21:40:18]

Add check back

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2530573002/diff/720001/base/metrics/field_tri...
base/metrics/field_trial.cc:829: if (!global_)
On 2016/11/29 20:19:27, Alexei Svitkine (slow) wrote:
> On 2016/11/29 19:52:44, lawrencewu wrote:
> > On 2016/11/29 19:32:47, Alexei Svitkine (slow) wrote:
> > > Does this actually get hit?
> > 
> > I added a log statement here and it doesn't get hit -- but I don't see the
> harm
> > in keeping this check.
> 
> I think it would be better to remove this if it's not necessary.

Actually, it looks like some tests do hit that and fail without it, so I added
the check back.

[commit-bot: I haz the power, 2016-11-29 21:40:55]

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

[Alexei Svitkine (slow), 2016-11-29 21:44:17]

OK, still lgtm

[commit-bot: I haz the power, 2016-11-29 22:47:20]

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

[commit-bot: I haz the power, 2016-11-29 22:47:21]

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...)

[lawrencewu, 2016-11-30 14:47:44]

jochen@: friendly ping

[jochen (gone - plz use gerrit), 2016-11-30 14:58:07]

lgtm

[lawrencewu, 2016-11-30 14:59:06]

The CQ bit was checked by lawrencewu@chromium.org

[commit-bot: I haz the power, 2016-11-30 14:59:30]

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, 2016-11-30 16:25:06]

CQ is committing da patch.

Bot data: {"patchset_id": 670009, "attempt_start_ts": 1480517946855550,
"parent_rev": "5f6000800c15418cc06896ce47a2e980ca175148", "commit_rev":
"ced4656b4c52011228f8ab4d72bc01e24a138489"}

[commit-bot: I haz the power, 2016-11-30 16:25:36]

Description was changed from

==========
Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

to

==========
Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

[commit-bot: I haz the power, 2016-11-30 16:25:38]

Committed patchset #41 (id:670009)

[commit-bot: I haz the power, 2016-11-30 16:28:41]

Description was changed from

==========
Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914
==========

to

==========
Share field trial allocator on zygote-using Linuxes

This passes a read-only handle to the field trial allocator on
Linux (that is, on POSIX machines that use the zygote model). It works
in the following way: in child_process_launcher.cc, we add our handle
to shared memory, which is a file descriptor (since shared memory is
really just an unnamed file), to files_to_register. The fds in this
mapping get remapped in the zygote process, and then all child
processes fork off the zygote, so the fds are the same in the child as
in the zygote. When we spawn a new process, then, we add our fd as a
switch on the command line (--field-trial-handle). Then, in the child
process, we must grab this fd and read from it. But first, we
have to get the correct fd using GlobalDescriptors, which is a
singleton that maps from the browser process's fd (key which we have)
to the remapped fd on the zygote (actual fd containing shared memory).
So we use that to look it up and then read from shared memory as
usual.

BUG=663914

Committed: https://crrev.com/c4fe88004d0457cf00b1731c1e974c96a3cd649e
Cr-Commit-Position: refs/heads/master@{#435284}
==========

[commit-bot: I haz the power, 2016-11-30 16:28:43]

Patchset 41 (id:??) landed as
https://crrev.com/c4fe88004d0457cf00b1731c1e974c96a3cd649e
Cr-Commit-Position: refs/heads/master@{#435284}