Initial implementation for sharing field trial state (win)

Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
Committed: https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f
Cr-Commit-Position: refs/heads/master@{#425013}

[lawrencewu, 2016-09-27 20:05:19]

Description was changed from

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
==========

to

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
==========

[lawrencewu, 2016-09-27 20:05:19]

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

[lawrencewu, 2016-09-27 20:07:55]

Initial CL for sharing field trial state via shared memory

still to do:
1) make this windows-specific, and fallback to original code if platform is
non-windows
2) write tests
3) plumb the shared_memory_handle flag properly

[Alexei Svitkine (slow), 2016-09-27 21:03:26]

Sweet!

Sounds like you have a good idea of the next steps involved - so I'll let
you work on them, but if there's a specific area you want my advice on, let
me know.

On Tue, Sep 27, 2016 at 4:07 PM, <lawrencewu@chromium.org> wrote:

> Reviewers: Alexei Svitkine (very slow)
> CL: https://codereview.chromium.org/2365273004/
>
> Message:
> Initial CL for sharing field trial state via shared memory
>
> still to do:
> 1) make this windows-specific, and fallback to original code if platform is
> non-windows
> 2) write tests
> 3) plumb the shared_memory_handle flag properly
>
> Description:
> Initial implementation for sharing field trial state (win)
>
> This plumbs the field trial state string from the gpu and renderer host
> processes and puts it in shared memory, passes the shared memory handle
> (and length) via a command-line flag to the child, and the child will
> initialize it there.
>
> BUG=131632
>
> Affected files (+56, -14 lines):
> M content/app/content_main_runner.cc
> M content/browser/browser_child_process_host_impl.h
> M content/browser/browser_child_process_host_impl.cc
> M content/browser/child_process_launcher.cc
> M content/browser/gpu/gpu_process_host.h
> M content/browser/gpu/gpu_process_host.cc
> M content/browser/renderer_host/render_process_host_impl.h
> M content/browser/renderer_host/render_process_host_impl.cc
>
>
> Index: content/app/content_main_runner.cc
> diff --git a/content/app/content_main_runner.cc
> b/content/app/content_main_runner.cc
> index 01bb5f00f42ce6a9499708f7a53c5766054c62fc..
> f915f94182fb3c4267dee37764446b8c69eff137 100644
> --- a/content/app/content_main_runner.cc
> +++ b/content/app/content_main_runner.cc
> @@ -26,12 +26,14 @@
> #include "base/logging.h"
> #include "base/macros.h"
> #include "base/memory/scoped_vector.h"
> +#include "base/memory/shared_memory.h"
> #include "base/metrics/field_trial.h"
> #include "base/metrics/histogram_base.h"
> #include "base/metrics/statistics_recorder.h"
> #include "base/path_service.h"
> #include "base/process/launch.h"
> #include "base/process/memory.h"
> +#include "base/process/process.h"
> #include "base/process/process_handle.h"
> #include "base/profiler/scoped_tracker.h"
> #include "base/strings/string_number_conversions.h"
> @@ -146,11 +148,24 @@ void InitializeFieldTrialAndFeatureList(
>
> // Ensure any field trials in browser are reflected into the child
> // process.
> - if (command_line.HasSwitch(switches::kForceFieldTrials)) {
> - bool result = base::FieldTrialList::CreateTrialsFromString(
> - command_line.GetSwitchValueASCII(switches::kForceFieldTrials),
> + if (command_line.HasSwitch("field_trial_handle")) {
> + int field_trial_handle = std::stoi(
> + command_line.GetSwitchValueASCII("field_trial_handle"));
> + size_t field_trial_length = std::stoi(
> + command_line.GetSwitchValueASCII("field_trial_length"));
> +
> + HANDLE handle = reinterpret_cast<HANDLE>(field_trial_handle);
> + base::SharedMemoryHandle shm_handle = base::SharedMemoryHandle(
> + handle, base::GetCurrentProcId());
> +
> + base::SharedMemory shared_memory(shm_handle, false);
> + shared_memory.Map(field_trial_length);
> +
> + char *field_trial_state = static_cast<char*>(shared_memory.memory());
> + bool result = base::FieldTrialList::CreateTrialsFromString(
> + std::string(field_trial_state),
> std::set<std::string>());
> - DCHECK(result);
> + DCHECK(result);
> }
>
> std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
> Index: content/browser/browser_child_process_host_impl.cc
> diff --git a/content/browser/browser_child_process_host_impl.cc
> b/content/browser/browser_child_process_host_impl.cc
> index e3eacd3ce01dead3132bee37df83760295395796..
> 441bd6598d1c3db7c30afd7086d90d2724025ec6 100644
> --- a/content/browser/browser_child_process_host_impl.cc
> +++ b/content/browser/browser_child_process_host_impl.cc
> @@ -208,7 +208,7 @@ void BrowserChildProcessHostImpl::TerminateAll() {
>
> // static
> void BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
> - base::CommandLine* cmd_line) {
> + base::CommandLine* cmd_line, base::SharedMemory* shared_memory) {
> std::string enabled_features;
> std::string disabled_features;
> base::FeatureList::GetInstance()->GetFeatureOverrides(&enabled_features,
> @@ -223,8 +223,16 @@ void BrowserChildProcessHostImpl::
> CopyFeatureAndFieldTrialFlags(
> std::string field_trial_states;
> base::FieldTrialList::AllStatesToString(&field_trial_states);
> if (!field_trial_states.empty()) {
> - cmd_line->AppendSwitchASCII(switches::kForceFieldTrials,
> - field_trial_states);
> + size_t length = field_trial_states.size() + 1;
> + shared_memory->CreateAndMapAnonymous(length);
> + memcpy(shared_memory->memory(), field_trial_states.c_str(), length);
> +
> + // HANDLE is just typedef'd to void *
> + HANDLE handle = shared_memory->handle().GetHandle();
> + auto uintptr_handle = reinterpret_cast<std::uintptr_t>(handle);
> + std::string string_handle = std::to_string(uintptr_handle);
> + cmd_line->AppendSwitchASCII("field_trial_handle", string_handle);
> + cmd_line->AppendSwitchASCII("field_trial_length",
> std::to_string(length));
> }
> }
>
> Index: content/browser/browser_child_process_host_impl.h
> diff --git a/content/browser/browser_child_process_host_impl.h
> b/content/browser/browser_child_process_host_impl.h
> index 43cd8bebc276fc86113f28bc3df96f6bb4824306..
> eadb5affbeb83a1c2699094140e32a55bdbe5906 100644
> --- a/content/browser/browser_child_process_host_impl.h
> +++ b/content/browser/browser_child_process_host_impl.h
> @@ -12,6 +12,7 @@
>
> #include "base/compiler_specific.h"
> #include "base/memory/ref_counted.h"
> +#include "base/memory/shared_memory.h"
> #include "base/memory/weak_ptr.h"
> #include "base/process/process.h"
> #include "base/single_thread_task_runner.h"
> @@ -65,7 +66,8 @@ class CONTENT_EXPORT BrowserChildProcessHostImpl
> // Copies kEnableFeatures and kDisableFeatures to the command line.
> Generates
> // them from the FeatureList override state, to take into account overrides
> // from FieldTrials.
> - static void CopyFeatureAndFieldTrialFlags(base::CommandLine* cmd_line);
> + static void CopyFeatureAndFieldTrialFlags(base::CommandLine* cmd_line,
> + base::SharedMemory* shared_memory);
>
> // BrowserChildProcessHost implementation:
> bool Send(IPC::Message* message) override;
> Index: content/browser/child_process_launcher.cc
> diff --git a/content/browser/child_process_launcher.cc
> b/content/browser/child_process_launcher.cc
> index 5a655368aa8bac219446a3ea3586a4c7b6eb7666..
> add6ed6088f20f972047f919883a716394c325e4 100644
> --- a/content/browser/child_process_launcher.cc
> +++ b/content/browser/child_process_launcher.cc
> @@ -155,6 +155,15 @@ void LaunchOnLauncherThread(const NotifyCallback&
> callback,
> cmd_line->AppendSwitchASCII(
> mojo::edk::PlatformChannelPair::kMojoPlatformChannelHandleSwitch,
> base::UintToString(base::win::HandleToUint32(handles[0])));
> +
> + // hack to get the shared memory handle to explicitly pass down to child
> + if (cmd_line->HasSwitch("field_trial_handle")) {
> + int ihandle = std::stoi(
> + cmd_line->GetSwitchValueASCII("field_trial_handle"));
> + HANDLE handle = reinterpret_cast<HANDLE>(ihandle);
> + handles.push_back(handle);
> + }
> +
> launch_result =
> StartSandboxedProcess(delegate, cmd_line, handles, &process);
> }
> Index: content/browser/gpu/gpu_process_host.cc
> diff --git a/content/browser/gpu/gpu_process_host.cc
> b/content/browser/gpu/gpu_process_host.cc
> index 367a69d9323fd065203a78416690ba86d09ad846..
> 3d509ec60b0a6625fe861a0342eb4ab923ceaf51 100644
> --- a/content/browser/gpu/gpu_process_host.cc
> +++ b/content/browser/gpu/gpu_process_host.cc
> @@ -974,7 +974,9 @@ bool
GpuProcessHost::LaunchGpuProcess(gpu::GpuPreferences*
> gpu_preferences) {
> base::CommandLine* cmd_line = new base::CommandLine(exe_path);
> #endif
> cmd_line->AppendSwitchASCII(switches::kProcessType,
> switches::kGpuProcess);
> - BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(cmd_line);
> + field_trial_state_.reset(new base::SharedMemory());
> + BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
> + cmd_line, field_trial_state_.get());
>
> #if defined(OS_WIN)
> cmd_line->AppendArg(switches::kPrefetchArgumentGpu);
> Index: content/browser/gpu/gpu_process_host.h
> diff --git a/content/browser/gpu/gpu_process_host.h
> b/content/browser/gpu/gpu_process_host.h
> index 9d6ee093a394dc60d6bb1bec7b7e926be02d6909..
> 00c888ea110f7512b88b5d9f6423ccfc492e4cdb 100644
> --- a/content/browser/gpu/gpu_process_host.h
> +++ b/content/browser/gpu/gpu_process_host.h
> @@ -288,6 +288,8 @@ class GpuProcessHost : public
> BrowserChildProcessHostDelegate,
>
> std::string shader_prefix_key_;
>
> + std::unique_ptr<base::SharedMemory> field_trial_state_;
> +
> DISALLOW_COPY_AND_ASSIGN(GpuProcessHost);
> };
>
> Index: content/browser/renderer_host/render_process_host_impl.cc
> diff --git a/content/browser/renderer_host/render_process_host_impl.cc
> b/content/browser/renderer_host/render_process_host_impl.cc
> index 00f88e99958af2563e24f2a1e09c9d1f1b39263c..
> b81e9bfb8064389bb3c540846cb2c106cafa0ddc 100644
> --- a/content/browser/renderer_host/render_process_host_impl.cc
> +++ b/content/browser/renderer_host/render_process_host_impl.cc
> @@ -1587,7 +1587,7 @@ static void
AppendCompositorCommandLineFlags(base::CommandLine*
> command_line) {
> }
>
> void RenderProcessHostImpl::AppendRendererCommandLine(
> - base::CommandLine* command_line) const {
> + base::CommandLine* command_line) {
> // Pass the process type first, so it shows first in process listings.
> command_line->AppendSwitchASCII(switches::kProcessType,
> switches::kRendererProcess);
> @@ -1626,7 +1626,7 @@ void RenderProcessHostImpl::
> AppendRendererCommandLine(
>
> void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
> const base::CommandLine& browser_cmd,
> - base::CommandLine* renderer_cmd) const {
> + base::CommandLine* renderer_cmd) {
> // Propagate the following switches to the renderer command line (along
> // with any associated values) if present in the browser command line.
> static const char* const kSwitchNames[] = {
> @@ -1839,7 +1839,9 @@ void RenderProcessHostImpl::
> PropagateBrowserCommandLineToRenderer(
> renderer_cmd->CopySwitchesFrom(browser_cmd, kSwitchNames,
> arraysize(kSwitchNames));
>
> - BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
> renderer_cmd);
> + field_trial_state_.reset(new base::SharedMemory());
> + BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(
> + renderer_cmd, field_trial_state_.get());
>
> if (browser_cmd.HasSwitch(switches::kTraceStartup) &&
> BrowserMainLoop::GetInstance()->is_tracing_startup_for_duration()) {
> Index: content/browser/renderer_host/render_process_host_impl.h
> diff --git a/content/browser/renderer_host/render_process_host_impl.h
> b/content/browser/renderer_host/render_process_host_impl.h
> index aac8afa60c469e67337e8ded3fe2a9e2d5abfcf8..
> 30fc4734f3f7116dee4bc89d6ef337f6e0e1abe7 100644
> --- a/content/browser/renderer_host/render_process_host_impl.h
> +++ b/content/browser/renderer_host/render_process_host_impl.h
> @@ -356,14 +356,14 @@ class CONTENT_EXPORT RenderProcessHostImpl
>
> // Generates a command line to be used to spawn a renderer and appends the
> // results to |*command_line|.
> - void AppendRendererCommandLine(base::CommandLine* command_line) const;
> + void AppendRendererCommandLine(base::CommandLine* command_line);
>
> // Copies applicable command line switches from the given |browser_cmd|
> line
> // flags to the output |renderer_cmd| line flags. Not all switches will be
> // copied over.
> void PropagateBrowserCommandLineToRenderer(
> const base::CommandLine& browser_cmd,
> - base::CommandLine* renderer_cmd) const;
> + base::CommandLine* renderer_cmd);
>
> // Inspects the current object state and sets/removes background priority
> if
> // appropriate. Should be called after any of the involved data members
> @@ -610,6 +610,8 @@ class CONTENT_EXPORT RenderProcessHostImpl
>
> base::WeakPtrFactory<RenderProcessHostImpl> weak_factory_;
>
> + std::unique_ptr<base::SharedMemory> field_trial_state_;
> +
> DISALLOW_COPY_AND_ASSIGN(RenderProcessHostImpl);
> };
>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lawrencewu, 2016-09-28 18:42:40]

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

[commit-bot: I haz the power, 2016-09-28 18:43: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-09-28 19:38:01]

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

[commit-bot: I haz the power, 2016-09-28 19:38:53]

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-09-28 20:21:31]

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

[commit-bot: I haz the power, 2016-09-28 20:21:32]

Dry run: This issue passed the CQ dry run.

[lawrencewu, 2016-09-29 18:13:43]

Finished all the todos above. Would like some guidance on how to deal with error
cases (what if Map fails, or creating the handle fails, etc). Should we fall
back? Also code is not super readable, especially with the windows macros. Maybe
we should split up functions even more?

[Alexei Svitkine (slow), 2016-09-30 21:47:52]

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

https://codereview.chromium.org/2365273004/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:972:
field_trial_state_.get()->handle().GetHandle(),
Can we pass base::SharedMemory* instead of HANDLE? Then, the code doesn't have
to be Windows specific and you can avoid a lot of ifdefs.

[lawrencewu, 2016-10-01 01:46:35]

Yep, good call. Patched that up, but didn't get a chance to compile on Windows
yet (will do on Monday).

[lawrencewu, 2016-10-03 13:48:28]

Patchset #8 (id:140001) has been deleted

[lawrencewu, 2016-10-03 14:14:21]

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

[commit-bot: I haz the power, 2016-10-03 14:14:29]

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

[lawrencewu, 2016-10-03 14:14:36]

Compiled and tested on Windows.

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

https://codereview.chromium.org/2365273004/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:972:
field_trial_state_.get()->handle().GetHandle(),
On 2016/09/30 21:47:52, Alexei Svitkine (slow) wrote:
> Can we pass base::SharedMemory* instead of HANDLE? Then, the code doesn't have
> to be Windows specific and you can avoid a lot of ifdefs.

Done.

[commit-bot: I haz the power, 2016-10-03 14:25:23]

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

[commit-bot: I haz the power, 2016-10-03 14:25:23]

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-10-03 14:43:20]

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

[commit-bot: I haz the power, 2016-10-03 14:43: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-10-03 15:11:06]

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

[commit-bot: I haz the power, 2016-10-03 15:11:10]

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

[Alexei Svitkine (slow), 2016-10-03 15:28:06]

https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browse...
File components/nacl/browser/nacl_broker_host_win.cc (right):

https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:78: NULL,
Nit: New code should use nullptr rather than NULL.

Please fix the other places where you're doing this too.

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

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:156:
command_line.GetSwitchValueASCII("field_trial_length"));
I suggest using a single flag whose value is the pair of these. Please define
this flag name in a *_switches.cc/.h file like we do for other flags. It can go
in content_switches.cc. I think.

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:167: std::string(field_trial_state),
When wrapping indent 4, not 2. You can possibly just run "git cl format" to
automatically fix this.

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:171: #else
Nit: ifdefs shouldn't be indented.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
No (c) for new code.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:19: };
If your test code has no use of the harness class, just remove the class and use
TEST() instead of TEST_F()

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:44: } // content
This should be "// namespace content". Also, put 2 spaces before the comment,
not 1.

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

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:157:
handles.push_back(field_trial_state->handle().GetHandle());
Since both of these functions are const, please pass the parameter to this
function as const as well.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:161: 
Nit: Remove extra added line.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:398: base::SharedMemory*
field_trial_state,
Suggest moving this further down - maybe after client? Things like command line
seems to be more important and should be earlier.

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

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1844: #if
defined(OS_WIN)
I think you can just ifdef the line below and have
CopyFeatureAndFieldTrialFlags() be called with both parameters on all platforms.
That function could then check if the shared memory is null or not to do the new
logic.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:590:
std::unique_ptr<base::SharedMemory> field_trial_state_;
Add a comment and document lifetime of this. Add a TODO to use the same shared
state between processes.

[lawrencewu, 2016-10-03 21:35:47]

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

[lawrencewu, 2016-10-03 21:36:11]

Addressed comments.

https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browse...
File components/nacl/browser/nacl_broker_host_win.cc (right):

https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browse...
components/nacl/browser/nacl_broker_host_win.cc:78: NULL,
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Nit: New code should use nullptr rather than NULL.
> 
> Please fix the other places where you're doing this too.

Done.

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

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:156:
command_line.GetSwitchValueASCII("field_trial_length"));
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> I suggest using a single flag whose value is the pair of these. Please define
> this flag name in a *_switches.cc/.h file like we do for other flags. It can
go
> in content_switches.cc. I think.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:167: std::string(field_trial_state),
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> When wrapping indent 4, not 2. You can possibly just run "git cl format" to
> automatically fix this.

Done. git cl format++

https://codereview.chromium.org/2365273004/diff/180001/content/app/content_ma...
content/app/content_main_runner.cc:171: #else
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Nit: ifdefs shouldn't be indented.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> No (c) for new code.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:19: };
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> If your test code has no use of the harness class, just remove the class and
use
> TEST() instead of TEST_F()

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:44: } // content
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> This should be "// namespace content". Also, put 2 spaces before the comment,
> not 1.

Done.

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

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:157:
handles.push_back(field_trial_state->handle().GetHandle());
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Since both of these functions are const, please pass the parameter to this
> function as const as well.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:161: 
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Nit: Remove extra added line.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/child_...
content/browser/child_process_launcher.cc:398: base::SharedMemory*
field_trial_state,
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Suggest moving this further down - maybe after client? Things like command
line
> seems to be more important and should be earlier.

Done.

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

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1844: #if
defined(OS_WIN)
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> I think you can just ifdef the line below and have
> CopyFeatureAndFieldTrialFlags() be called with both parameters on all
platforms.
> That function could then check if the shared memory is null or not to do the
new
> logic.

Done.

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2365273004/diff/180001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:590:
std::unique_ptr<base::SharedMemory> field_trial_state_;
On 2016/10/03 15:28:05, Alexei Svitkine (slow) wrote:
> Add a comment and document lifetime of this. Add a TODO to use the same shared
> state between processes.

Done.

[commit-bot: I haz the power, 2016-10-03 21:37:01]

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-10-03 21:50:38]

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

[commit-bot: I haz the power, 2016-10-03 21:50:39]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-10-04 14:33:57]

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

[commit-bot: I haz the power, 2016-10-04 14:34:14]

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-10-04 14:45:44]

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

[commit-bot: I haz the power, 2016-10-04 14:45:45]

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-10-04 14:58:49]

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

[commit-bot: I haz the power, 2016-10-04 14:59:02]

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-10-04 15:13:25]

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

[commit-bot: I haz the power, 2016-10-04 15:13:26]

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

[lawrencewu, 2016-10-04 15:41:10]

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

[commit-bot: I haz the power, 2016-10-04 15:41:21]

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-10-04 16:45:32]

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

[commit-bot: I haz the power, 2016-10-04 16:45:33]

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-10-04 17:42:24]

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

[commit-bot: I haz the power, 2016-10-04 17:43: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-10-04 18:20:59]

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

[commit-bot: I haz the power, 2016-10-04 18:21:25]

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-10-04 18:49:36]

https://codereview.chromium.org/2365273004/diff/220001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/220001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:12: #include
"content/browser/browser_child_process_host_impl.h"
This should be the first include.

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

https://codereview.chromium.org/2365273004/diff/310001/content/app/content_ma...
content/app/content_main_runner.cc:153: std::string arg =
Suggest moving this code to base/metrics. We can have a static FieldTrialList
static method that is CreateTrialsFromCommandLine().

Then, this logic can be inside there - to either use the handle or the force
flag.

Also, I suggest not having ifdefs around this part assuming the code compiles on
non-Windows platforms.

https://codereview.chromium.org/2365273004/diff/310001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2365273004/diff/310001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:229: if (shared_memory) {
I also suggest putting this code in base/metrics as well. The function would be
responsible for adding the data to the command line.

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:982:
features::kShareFieldTrialStateViaSharedMemory))
Nit: {}'s

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:988: if
(!field_trial_state_.get()->handle().GetHandle())
Is this just an error case? Needs a comment at the very least.

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

https://codereview.chromium.org/2365273004/diff/310001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1845:
field_trial_state_.reset(new base::SharedMemory());
I don't like duplicating this code - especially since it has to have ifdefs.

Instead, pass a pointer to the scoped ptr to the function, so it can do the
.reset() itself.

e.g.

BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(renderer_cmd,
&field_trial_state_);

https://codereview.chromium.org/2365273004/diff/310001/content/public/browser...
File content/public/browser/browser_child_process_host.h (right):

https://codereview.chromium.org/2365273004/diff/310001/content/public/browser...
content/public/browser/browser_child_process_host.h:66: // Takes ownership of
|cmd_line| and |delegate|.
Document the new param.

[commit-bot: I haz the power, 2016-10-04 20:26:16]

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

[commit-bot: I haz the power, 2016-10-04 20:26:17]

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-10-05 20:57:24]

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

[lawrencewu, 2016-10-05 20:57:57]

Addressed comments.

https://codereview.chromium.org/2365273004/diff/220001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/220001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:12: #include
"content/browser/browser_child_process_host_impl.h"
On 2016/10/04 18:49:35, Alexei Svitkine (slow) wrote:
> This should be the first include.

Done.

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

https://codereview.chromium.org/2365273004/diff/310001/content/app/content_ma...
content/app/content_main_runner.cc:153: std::string arg =
On 2016/10/04 18:49:35, Alexei Svitkine (slow) wrote:
> Suggest moving this code to base/metrics. We can have a static FieldTrialList
> static method that is CreateTrialsFromCommandLine().
> 
> Then, this logic can be inside there - to either use the handle or the force
> flag.
> 
> Also, I suggest not having ifdefs around this part assuming the code compiles
on
> non-Windows platforms.

Done. Regarding the ifdefs, I think they're necessary because HANDLE is a
windows-specific type. In the future, the ifdef's scope will be reduced (so each
platform just encodes the handle/fd their own specific way), but for now I think
we should keep them outside the if of the switch.

https://codereview.chromium.org/2365273004/diff/310001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2365273004/diff/310001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:229: if (shared_memory) {
On 2016/10/04 18:49:35, Alexei Svitkine (slow) wrote:
> I also suggest putting this code in base/metrics as well. The function would
be
> responsible for adding the data to the command line.

Done.

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:982:
features::kShareFieldTrialStateViaSharedMemory))
On 2016/10/04 18:49:35, Alexei Svitkine (slow) wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/2365273004/diff/310001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:988: if
(!field_trial_state_.get()->handle().GetHandle())
On 2016/10/04 18:49:36, Alexei Svitkine (slow) wrote:
> Is this just an error case? Needs a comment at the very least.

Yes, it is the case when there were no field trials (happens in browsertests).
The handle will still try to get passed down but won't work because it's
invalid. With the new changes we don't need to do this anymore, it's implicit in
the flow.

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

https://codereview.chromium.org/2365273004/diff/310001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1845:
field_trial_state_.reset(new base::SharedMemory());
On 2016/10/04 18:49:36, Alexei Svitkine (slow) wrote:
> I don't like duplicating this code - especially since it has to have ifdefs.
> 
> Instead, pass a pointer to the scoped ptr to the function, so it can do the
> .reset() itself.
> 
> e.g.
> 
> BrowserChildProcessHostImpl::CopyFeatureAndFieldTrialFlags(renderer_cmd,
> &field_trial_state_);

Done.

https://codereview.chromium.org/2365273004/diff/310001/content/public/browser...
File content/public/browser/browser_child_process_host.h (right):

https://codereview.chromium.org/2365273004/diff/310001/content/public/browser...
content/public/browser/browser_child_process_host.h:66: // Takes ownership of
|cmd_line| and |delegate|.
On 2016/10/04 18:49:36, Alexei Svitkine (slow) wrote:
> Document the new param.

Done.

[commit-bot: I haz the power, 2016-10-05 20:58:14]

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-10-05 21:03:16]

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

[commit-bot: I haz the power, 2016-10-05 21:03:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexei Svitkine (slow), 2016-10-06 13:45:30]

Looks great - almost there - I think this should be the last major round of
comments.

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

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:570: #if defined(OS_WIN)
Ifdef should be around the if.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:588: }
Add a return statement to the block above.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:601: base::CommandLine* cmd_line,
Non-const params should be last.

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

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.h:469: // If using shared memory to pass around the
list of field trials, then
The first sentence should describe what the function does. Then, you can go into
details of how the args work.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.h:477: const base::CommandLine cmd_line,
Pass by const& - otherwise you're copying the object. Here and in the .cc file.

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:227: if
(base::FeatureList::IsEnabled(
Can this whole logic live in the helper function (including the old path)? Then,
the feature can move to base too and can probably just be declared inside the
field trials .cc file and not exposed elsewhere.

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:43: }
Can this test now move to base/ ? Or is there still something in
browser_child_process_host_impl that needs to be tested?

[lawrencewu, 2016-10-06 14:00:45]

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

[commit-bot: I haz the power, 2016-10-06 14:00:58]

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-10-06 14:03:43]

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

[commit-bot: I haz the power, 2016-10-06 14:03:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lawrencewu, 2016-10-06 15:47:15]

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

[commit-bot: I haz the power, 2016-10-06 15:47:43]

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-10-06 15:50:42]

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

[commit-bot: I haz the power, 2016-10-06 15:50:43]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lawrencewu, 2016-10-06 20:13:20]

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

[commit-bot: I haz the power, 2016-10-06 20:14:02]

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

[lawrencewu, 2016-10-06 20:14:58]

Addressed comments and merged with origin/master.

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

https://codereview.chromium.org/2365273004/diff/310001/content/app/content_ma...
content/app/content_main_runner.cc:153: std::string arg =
On 2016/10/04 18:49:35, Alexei Svitkine (slow) wrote:
> Suggest moving this code to base/metrics. We can have a static FieldTrialList
> static method that is CreateTrialsFromCommandLine().
> 
> Then, this logic can be inside there - to either use the handle or the force
> flag.
> 
> Also, I suggest not having ifdefs around this part assuming the code compiles
on
> non-Windows platforms.

Done. Regarding the ifdefs, I think they're necessary because HANDLE is a
windows-specific type. In the future, the ifdef's scope will be reduced (so each
platform just encodes the handle/fd their own specific way), but for now I think
we need them here.

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

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:570: #if defined(OS_WIN)
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Ifdef should be around the if.

Done.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:588: }
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Add a return statement to the block above.

Done.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.cc:601: base::CommandLine* cmd_line,
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Non-const params should be last.

Done.

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

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.h:469: // If using shared memory to pass around the
list of field trials, then
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> The first sentence should describe what the function does. Then, you can go
into
> details of how the args work.

Done.

https://codereview.chromium.org/2365273004/diff/330001/base/metrics/field_tri...
base/metrics/field_trial.h:477: const base::CommandLine cmd_line,
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Pass by const& - otherwise you're copying the object. Here and in the .cc
file.

Done.

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:227: if
(base::FeatureList::IsEnabled(
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Can this whole logic live in the helper function (including the old path)?
Then,
> the feature can move to base too and can probably just be declared inside the
> field trials .cc file and not exposed elsewhere.

Done.

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
File content/browser/browser_child_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/2365273004/diff/330001/content/browser/browse...
content/browser/browser_child_process_host_impl_unittest.cc:43: }
On 2016/10/06 13:45:30, Alexei Svitkine (slow) wrote:
> Can this test now move to base/ ? Or is there still something in
> browser_child_process_host_impl that needs to be tested?

Done.

[commit-bot: I haz the power, 2016-10-06 20:18:57]

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

[commit-bot: I haz the power, 2016-10-06 20:18:58]

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_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lawrencewu, 2016-10-06 20:30:49]

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

[commit-bot: I haz the power, 2016-10-06 20:32: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, 2016-10-06 20:41:16]

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

[commit-bot: I haz the power, 2016-10-06 20:41:18]

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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[lawrencewu, 2016-10-06 20:48:33]

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

[commit-bot: I haz the power, 2016-10-06 20:48:59]

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-10-06 21:23:37]

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

[commit-bot: I haz the power, 2016-10-06 21:23:41]

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

[Alexei Svitkine (slow), 2016-10-06 21:39:29]

LGTM % comments

You'll need to add some owners now. :)

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

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:36: "ShareFieldTrialStateViaSharedMemory",
base::FEATURE_DISABLED_BY_DEFAULT};
Nit: How about "UseSharedMemoryForFieldTrials" - a bit shorter?

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:578: int field_trial_length =
std::stoi(arg.substr(token + 1, arg.length()));
Do you need to do any kind of error handling here?

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:584: base::SharedMemory shared_memory(shm_handle,
false);
Add a comment that this will be deleted when this goes out of scope but that's
OK because we need it only for the duration of the CreateTrialsFromString() call
currently.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:613: #if defined(OS_WIN)
Put the whole if under ifdef and remove the else - just have it follow the block
- so that on non-Windows it simply doesn't check the feature and executes the
--force-fieldtrials code path.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:617: memcpy(shm->memory(),
field_trial_states.c_str(), length);
Is it guaranteed to be null terminated? If not, please do it explicitly.

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

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1158:
static_cast<char*>(field_trial_state.get()->memory()));
Nit: Just inline this into the EXPECT_EQ

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

https://codereview.chromium.org/2365273004/diff/430001/content/app/content_ma...
content/app/content_main_runner.cc:35: #include "base/process/process.h"
Is this include needed?

https://codereview.chromium.org/2365273004/diff/430001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2365273004/diff/430001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:51: #include
"content/public/common/content_features.h"
Nit: Remove.

https://codereview.chromium.org/2365273004/diff/430001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2365273004/diff/430001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:563: // object across
processes.
Suggest filing a crbug for this later work and referencing it in this comment.
Same for the other place you have this TODO.

[lawrencewu, 2016-10-07 15:07:19]

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

[lawrencewu, 2016-10-07 15:07:21]

Addressed asvitkine's comments.

@jam, would you mind looking over the code in content/ and components/?

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

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:36: "ShareFieldTrialStateViaSharedMemory",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Nit: How about "UseSharedMemoryForFieldTrials" - a bit shorter?

Done.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:578: int field_trial_length =
std::stoi(arg.substr(token + 1, arg.length()));
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Do you need to do any kind of error handling here?

I thought about it, and I think it is unnecessary -- if the command line has the
handle switch, then it must be the case that it was added by
CopyFieldTrialStateToFlags(), so there will be a ',' separated by two integers
in the flag param.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:584: base::SharedMemory shared_memory(shm_handle,
false);
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Add a comment that this will be deleted when this goes out of scope but that's
> OK because we need it only for the duration of the CreateTrialsFromString()
call
> currently.

Done.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:613: #if defined(OS_WIN)
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Put the whole if under ifdef and remove the else - just have it follow the
block
> - so that on non-Windows it simply doesn't check the feature and executes the
> --force-fieldtrials code path.

Done.

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial.cc:617: memcpy(shm->memory(),
field_trial_states.c_str(), length);
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Is it guaranteed to be null terminated? If not, please do it explicitly.

c_str() is guaranteed to be null terminated: http://stackoverflow.com/a/12697815

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

https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:1158:
static_cast<char*>(field_trial_state.get()->memory()));
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Nit: Just inline this into the EXPECT_EQ

Done.

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

https://codereview.chromium.org/2365273004/diff/430001/content/app/content_ma...
content/app/content_main_runner.cc:35: #include "base/process/process.h"
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Is this include needed?

Nope. Removed.

https://codereview.chromium.org/2365273004/diff/430001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2365273004/diff/430001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:51: #include
"content/public/common/content_features.h"
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Nit: Remove.

Done.

https://codereview.chromium.org/2365273004/diff/430001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2365273004/diff/430001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:563: // object across
processes.
On 2016/10/06 21:39:28, Alexei Svitkine (slow) wrote:
> Suggest filing a crbug for this later work and referencing it in this comment.
> Same for the other place you have this TODO.

Done: crbug.com/653874

[lawrencewu, 2016-10-07 15:34:09]

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

[commit-bot: I haz the power, 2016-10-07 15:34:23]

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-10-07 16:38:34]

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

[commit-bot: I haz the power, 2016-10-07 16:38:35]

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-10-07 17:12:45]

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

[commit-bot: I haz the power, 2016-10-07 17:13:00]

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-10-07 18:11:59]

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

[commit-bot: I haz the power, 2016-10-07 18:12:00]

Dry run: This issue passed the CQ dry run.

[jam, 2016-10-10 15:08:41]

A few questions
1) it seems gratuitous that we have a field trial for this. i.e. sending a
string in shared memory doesn't seem like it's something that'll change
depending on client machines. why not just add an integration test that tests
that this is working as expected
2) why is the initial implementation windows only?
3) why is the initial implementation creating shared memory segment for each
child process instead of sharing it?

[lawrencewu, 2016-10-10 18:13:48]

On 2016/10/10 15:08:41, jam wrote:
> A few questions
> 1) it seems gratuitous that we have a field trial for this. i.e. sending a
> string in shared memory doesn't seem like it's something that'll change
> depending on client machines. why not just add an integration test that tests
> that this is working as expected
> 2) why is the initial implementation windows only?
> 3) why is the initial implementation creating shared memory segment for each
> child process instead of sharing it?

Hey jam, happy to answer:

1) Sorry, not quite sure what you mean here, did you mean unit test?
2) This is kind of an MVP, and since Windows is the most used platform and also
the platform on which we are most likely to hit the command-line length limit,
we (asvitkine and I) decided to implement it for Windows first. I will be
submitting CLs for the other platforms later. You can see the design doc with
the schedule for this project here:
https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...
3) Similar reason as 2. I'm working on having one single shared memory segment
across all processes, but that entails some yak shaving and I don't want this CL
to be too big. My next CL will address this.

[lawrencewu, 2016-10-10 18:13:51]

On 2016/10/10 15:08:41, jam wrote:
> A few questions
> 1) it seems gratuitous that we have a field trial for this. i.e. sending a
> string in shared memory doesn't seem like it's something that'll change
> depending on client machines. why not just add an integration test that tests
> that this is working as expected
> 2) why is the initial implementation windows only?
> 3) why is the initial implementation creating shared memory segment for each
> child process instead of sharing it?

Hey jam, happy to answer:

1) Sorry, not quite sure what you mean here, did you mean unit test?
2) This is kind of an MVP, and since Windows is the most used platform and also
the platform on which we are most likely to hit the command-line length limit,
we (asvitkine and I) decided to implement it for Windows first. I will be
submitting CLs for the other platforms later. You can see the design doc with
the schedule for this project here:
https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...
3) Similar reason as 2. I'm working on having one single shared memory segment
across all processes, but that entails some yak shaving and I don't want this CL
to be too big. My next CL will address this.

[jam, 2016-10-10 21:10:07]

On 2016/10/10 18:13:51, lawrencewu wrote:
> On 2016/10/10 15:08:41, jam wrote:
> > A few questions
> > 1) it seems gratuitous that we have a field trial for this. i.e. sending a
> > string in shared memory doesn't seem like it's something that'll change
> > depending on client machines. why not just add an integration test that
tests
> > that this is working as expected
> > 2) why is the initial implementation windows only?
> > 3) why is the initial implementation creating shared memory segment for each
> > child process instead of sharing it?
> 
> Hey jam, happy to answer:
> 
> 1) Sorry, not quite sure what you mean here, did you mean unit test?

I mean why is there a field trial for switching to use shared memory, instead of
just flipping over in one cl? Why do we need experimentation from the field for
this? see kUseSharedMemoryForFieldTrials

> 2) This is kind of an MVP, and since Windows is the most used platform and
also
> the platform on which we are most likely to hit the command-line length limit,
> we (asvitkine and I) decided to implement it for Windows first. I will be
> submitting CLs for the other platforms later. You can see the design doc with
> the schedule for this project here:
>
https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...

My question was whether it saves much effort to do it for other platforms first,
i.e. shouldn't our platform abstractions make this simple to do everywhere?

> 3) Similar reason as 2. I'm working on having one single shared memory segment
> across all processes, but that entails some yak shaving and I don't want this
CL
> to be too big. My next CL will address this.

Curious what you're referring to?

[lawrencewu, 2016-10-11 13:46:28]

On 2016/10/10 21:10:07, jam wrote:
> On 2016/10/10 18:13:51, lawrencewu wrote:
> > On 2016/10/10 15:08:41, jam wrote:
> > > A few questions
> > > 1) it seems gratuitous that we have a field trial for this. i.e. sending a
> > > string in shared memory doesn't seem like it's something that'll change
> > > depending on client machines. why not just add an integration test that
> tests
> > > that this is working as expected
> > > 2) why is the initial implementation windows only?
> > > 3) why is the initial implementation creating shared memory segment for
each
> > > child process instead of sharing it?
> > 
> > Hey jam, happy to answer:
> > 
> > 1) Sorry, not quite sure what you mean here, did you mean unit test?
> 
> I mean why is there a field trial for switching to use shared memory, instead
of
> just flipping over in one cl? Why do we need experimentation from the field
for
> this? see kUseSharedMemoryForFieldTrials
> 
> > 2) This is kind of an MVP, and since Windows is the most used platform and
> also
> > the platform on which we are most likely to hit the command-line length
limit,
> > we (asvitkine and I) decided to implement it for Windows first. I will be
> > submitting CLs for the other platforms later. You can see the design doc
with
> > the schedule for this project here:
> >
>
https://docs.google.com/document/d/1CTFOpOpbqnOTzo_gCuSyqDur0AeY2X_LFayylN_oH...
> 
> My question was whether it saves much effort to do it for other platforms
first,
> i.e. shouldn't our platform abstractions make this simple to do everywhere?
> 
> > 3) Similar reason as 2. I'm working on having one single shared memory
segment
> > across all processes, but that entails some yak shaving and I don't want
this
> CL
> > to be too big. My next CL will address this.
> 
> Curious what you're referring to?

1) We want to collect results in the wild from this because I am not totally
sure how client machines will handle this change. There are some potential
things that can go wrong, such as clients not inheriting the handle properly. We
also eventually want to measure things like memory usage and performance so I
think it is just safer if we place this behind a feature flag, so we can track
how the change is doing in the wild.

2) Unfortunately there isn't currently an abstraction for
serializing/deserializing a shared memory handle, which is what I am doing, so
there is some extra effort involved in porting it to other platforms. For
example, the permissions system and the shared memory API differs per platform
(on Windows, I have to manually inherit the handle, for example).

3) Now that I think about it, you are right in that it probably won't be that
much extra work in changing it to use only one memory segment -- I could just
add the data member to the FieldTrialList singleton. However, given that I am
already doing this in the next CL (as well as changing the data format of the
shared memory), I'm wondering if you can just take a look at this CL as-is.

[Alexei Svitkine (slow), 2016-10-11 15:27:35]

Just want to expand on the question about using a base::Feature:

Since this affects child process creation, we wanted to make sure there's no
performance or stability regression as a result of this change, so the best way
to test this is with a 50/50 trial. I think this can use the new lightweight
process and we don't even need to go to beta or stable - just sanity check the
data from canary and possibly dev.

The other reason is I don't think this intermediate state, where we create
multiple shared memory segments, should be turned on by default yet. I think it
should be turned on by default when we can share the segment between all the
child processes, which is what Lawrence is working on next. So having it behind
a base::Feature until then makes sense to me.

As to why not wait for the next CL and have both at once? The next CL actually
involves changing the format of the content of the shared memory (because the
current simple string can't be shared trivially because there are some
synchronization issues as it can change between launches). And it's better to
have smaller incremental CLs that are easier to review, then one big CL at the
end - especially for an internship project which this is.

[jam, 2016-10-11 15:40:44]

Thanks for the explanations. It's fine to land in stages then (I was previously
wondering if it would be roughly the same size with one shared memory and cross
platform, but it looks like the answer is no).

However I'm not a fan of using finch flags for this. We change internals of
content all the time, including process launching. I do not want us to use
experiments for every change because that increases complexity and slows us
down. So this code path can be disabled by a boolean that is set locally until
the improvements cited land, but I don't want this to be launched using a finch
experiment. The code here is comparatively little and we should be able to
verify that this doesn't cause performance regressions from our existing perf
bots. Similarly the code path is small and always runs, so we should know that
if it has crashes through our existing bots.

On 2016/10/11 15:27:35, Alexei Svitkine (slow) wrote:
> Just want to expand on the question about using a base::Feature:
> 
> Since this affects child process creation, we wanted to make sure there's no
> performance or stability regression as a result of this change, so the best
way
> to test this is with a 50/50 trial. I think this can use the new lightweight
> process and we don't even need to go to beta or stable - just sanity check the
> data from canary and possibly dev.
> 
> The other reason is I don't think this intermediate state, where we create
> multiple shared memory segments, should be turned on by default yet. I think
it
> should be turned on by default when we can share the segment between all the
> child processes, which is what Lawrence is working on next. So having it
behind
> a base::Feature until then makes sense to me.
> 
> As to why not wait for the next CL and have both at once? The next CL actually
> involves changing the format of the content of the shared memory (because the
> current simple string can't be shared trivially because there are some
> synchronization issues as it can change between launches). And it's better to
> have smaller incremental CLs that are easier to review, then one big CL at the
> end - especially for an internship project which this is.

[lawrencewu, 2016-10-11 16:13:37]

On 2016/10/11 15:40:44, jam wrote:
> Thanks for the explanations. It's fine to land in stages then (I was
previously
> wondering if it would be roughly the same size with one shared memory and
cross
> platform, but it looks like the answer is no).
> 
> However I'm not a fan of using finch flags for this. We change internals of
> content all the time, including process launching. I do not want us to use
> experiments for every change because that increases complexity and slows us
> down. So this code path can be disabled by a boolean that is set locally until
> the improvements cited land, but I don't want this to be launched using a
finch
> experiment. The code here is comparatively little and we should be able to
> verify that this doesn't cause performance regressions from our existing perf
> bots. Similarly the code path is small and always runs, so we should know that
> if it has crashes through our existing bots.
> 
> On 2016/10/11 15:27:35, Alexei Svitkine (slow) wrote:
> > Just want to expand on the question about using a base::Feature:
> > 
> > Since this affects child process creation, we wanted to make sure there's no
> > performance or stability regression as a result of this change, so the best
> way
> > to test this is with a 50/50 trial. I think this can use the new lightweight
> > process and we don't even need to go to beta or stable - just sanity check
the
> > data from canary and possibly dev.
> > 
> > The other reason is I don't think this intermediate state, where we create
> > multiple shared memory segments, should be turned on by default yet. I think
> it
> > should be turned on by default when we can share the segment between all the
> > child processes, which is what Lawrence is working on next. So having it
> behind
> > a base::Feature until then makes sense to me.
> > 
> > As to why not wait for the next CL and have both at once? The next CL
actually
> > involves changing the format of the content of the shared memory (because
the
> > current simple string can't be shared trivially because there are some
> > synchronization issues as it can change between launches). And it's better
to
> > have smaller incremental CLs that are easier to review, then one big CL at
the
> > end - especially for an internship project which this is.

OK, I will change it to use a local boolean.

[lawrencewu, 2016-10-11 17:24:03]

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

[commit-bot: I haz the power, 2016-10-11 17:24: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, 2016-10-11 17:40:07]

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

[commit-bot: I haz the power, 2016-10-11 17:40:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lawrencewu, 2016-10-12 13:48:10]

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

[commit-bot: I haz the power, 2016-10-12 13:48:24]

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-10-12 13:52:34]

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

[commit-bot: I haz the power, 2016-10-12 13:52:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lawrencewu, 2016-10-12 14:09:56]

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

[commit-bot: I haz the power, 2016-10-12 14:10:05]

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-10-12 14:20:36]

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

[commit-bot: I haz the power, 2016-10-12 14:20:37]

Dry run: Exceeded global retry quota

[lawrencewu, 2016-10-12 15:44:28]

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

[commit-bot: I haz the power, 2016-10-12 15:44: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, 2016-10-12 17:02:10]

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

[commit-bot: I haz the power, 2016-10-12 17:02:11]

Dry run: This issue passed the CQ dry run.

[lawrencewu, 2016-10-12 17:31:01]

@jam: changed it to use a bool instead of a base::Feature, mind taking a look?

[jam, 2016-10-12 21:18:20]

lgtm

[lawrencewu, 2016-10-13 13:48:39]

The CQ bit was checked by lawrencewu@chromium.org

[lawrencewu, 2016-10-13 13:48:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2365273004/#ps530001
(title: "rebase + gclient sync")

[commit-bot: I haz the power, 2016-10-13 13:48:53]

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-10-13 13:54:51]

Description was changed from

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
==========

to

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
==========

[commit-bot: I haz the power, 2016-10-13 13:54:53]

Committed patchset #27 (id:530001)

[commit-bot: I haz the power, 2016-10-13 13:56:36]

Description was changed from

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632
==========

to

==========
Initial implementation for sharing field trial state (win)

This plumbs the field trial state string from the gpu and renderer host
processes and puts it in shared memory, passes the shared memory handle
(and length) via a command-line flag to the child, and the child will
initialize it there.

BUG=131632

Committed: https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f
Cr-Commit-Position: refs/heads/master@{#425013}
==========

[commit-bot: I haz the power, 2016-10-13 13:56:38]

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