Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2365273004: Initial implementation for sharing field trial state (win) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by lawrencewu
Modified:
4 months, 1 week ago
CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, gavinp+memory_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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 Committed: https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f Cr-Commit-Position: refs/heads/master@{#425013}

Patch Set 1 #

Patch Set 2 : Revert shared_memory_win.cc #

Patch Set 3 : Remove comment #

Patch Set 4 : Fallback to original behavior on non-Windows, plumb handle properly #

Patch Set 5 : Cleanup #

Patch Set 6 : Move field_trial_state_ so it's not last in class #

Patch Set 7 : Write unit test for BrowserChildProcessHostImpl #

Total comments: 2

Patch Set 8 : Pass base::SharedMemory* instead of HANDLE #

Patch Set 9 : Remove windows macro causing compilation issue #

Total comments: 24

Patch Set 10 : Address comments #

Patch Set 11 : Fix unit test and remove macro #

Total comments: 2

Patch Set 12 : fix compilation issue #

Patch Set 13 : fix another compilation issue #

Patch Set 14 : fix unittest #

Patch Set 15 : fix failing browsertests #

Patch Set 16 : add feature logic for sharing field trials #

Total comments: 13

Patch Set 17 : Address comments #

Total comments: 14

Patch Set 18 : gclient sync #

Patch Set 19 : Address comments #

Patch Set 20 : merge #

Patch Set 21 : remove unused file #

Patch Set 22 : add missing include #

Total comments: 18

Patch Set 23 : address comments #

Patch Set 24 : fix unittest #

Patch Set 25 : use boolean instead of feature #

Patch Set 26 : merge #

Patch Set 27 : rebase + gclient sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -59 lines) Patch
M base/metrics/field_trial.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +24 lines, -0 lines 0 comments Download
M base/metrics/field_trial.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +82 lines, -0 lines 0 comments Download
M base/metrics/field_trial_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +19 lines, -0 lines 0 comments Download
M components/nacl/browser/nacl_broker_host_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -3 lines 0 comments Download
M components/nacl/browser/nacl_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -2 lines 0 comments Download
M content/app/content_main_runner.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +3 lines, -6 lines 0 comments Download
M content/browser/browser_child_process_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/browser_child_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 4 chunks +7 lines, -12 lines 0 comments Download
M content/browser/child_process_launcher.h View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -1 line 0 comments Download
M content/browser/child_process_launcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 6 chunks +10 lines, -6 lines 0 comments Download
M content/browser/gpu/gpu_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +6 lines, -5 lines 0 comments Download
M content/browser/ppapi_plugin_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +3 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +8 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 4 chunks +5 lines, -4 lines 0 comments Download
M content/browser/utility_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +4 lines, -7 lines 0 comments Download
M content/public/browser/browser_child_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -5 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +6 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 119 (92 generated)
lawrencewu
Initial CL for sharing field trial state via shared memory still to do: 1) make ...
4 months, 3 weeks ago (2016-09-27 20:07:55 UTC) #3
Alexei Svitkine (slow)
Sweet! Sounds like you have a good idea of the next steps involved - so ...
4 months, 3 weeks ago (2016-09-27 21:03:26 UTC) #4
lawrencewu
Finished all the todos above. Would like some guidance on how to deal with error ...
4 months, 3 weeks ago (2016-09-29 18:13:43 UTC) #11
Alexei Svitkine (slow)
https://codereview.chromium.org/2365273004/diff/120001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2365273004/diff/120001/content/browser/renderer_host/render_process_host_impl.cc#newcode972 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, ...
4 months, 3 weeks ago (2016-09-30 21:47:52 UTC) #12
lawrencewu
Yep, good call. Patched that up, but didn't get a chance to compile on Windows ...
4 months, 3 weeks ago (2016-10-01 01:46:35 UTC) #13
lawrencewu
Compiled and tested on Windows. https://codereview.chromium.org/2365273004/diff/120001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2365273004/diff/120001/content/browser/renderer_host/render_process_host_impl.cc#newcode972 content/browser/renderer_host/render_process_host_impl.cc:972: field_trial_state_.get()->handle().GetHandle(), On 2016/09/30 21:47:52, ...
4 months, 2 weeks ago (2016-10-03 14:14:36 UTC) #17
Alexei Svitkine (slow)
https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browser/nacl_broker_host_win.cc File components/nacl/browser/nacl_broker_host_win.cc (right): https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browser/nacl_broker_host_win.cc#newcode78 components/nacl/browser/nacl_broker_host_win.cc:78: NULL, Nit: New code should use nullptr rather than ...
4 months, 2 weeks ago (2016-10-03 15:28:06 UTC) #24
lawrencewu
Addressed comments. https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browser/nacl_broker_host_win.cc File components/nacl/browser/nacl_broker_host_win.cc (right): https://codereview.chromium.org/2365273004/diff/180001/components/nacl/browser/nacl_broker_host_win.cc#newcode78 components/nacl/browser/nacl_broker_host_win.cc:78: NULL, On 2016/10/03 15:28:05, Alexei Svitkine (slow) ...
4 months, 2 weeks ago (2016-10-03 21:36:11 UTC) #26
Alexei Svitkine (slow)
https://codereview.chromium.org/2365273004/diff/220001/content/browser/browser_child_process_host_impl_unittest.cc File content/browser/browser_child_process_host_impl_unittest.cc (right): https://codereview.chromium.org/2365273004/diff/220001/content/browser/browser_child_process_host_impl_unittest.cc#newcode12 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_main_runner.cc ...
4 months, 2 weeks ago (2016-10-04 18:49:36 UTC) #46
lawrencewu
Addressed comments. https://codereview.chromium.org/2365273004/diff/220001/content/browser/browser_child_process_host_impl_unittest.cc File content/browser/browser_child_process_host_impl_unittest.cc (right): https://codereview.chromium.org/2365273004/diff/220001/content/browser/browser_child_process_host_impl_unittest.cc#newcode12 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 ...
4 months, 2 weeks ago (2016-10-05 20:57:57 UTC) #50
Alexei Svitkine (slow)
Looks great - almost there - I think this should be the last major round ...
4 months, 2 weeks ago (2016-10-06 13:45:30 UTC) #54
lawrencewu
Addressed comments and merged with origin/master. https://codereview.chromium.org/2365273004/diff/310001/content/app/content_main_runner.cc File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2365273004/diff/310001/content/app/content_main_runner.cc#newcode153 content/app/content_main_runner.cc:153: std::string arg = ...
4 months, 2 weeks ago (2016-10-06 20:14:58 UTC) #65
Alexei Svitkine (slow)
LGTM % comments You'll need to add some owners now. :) https://codereview.chromium.org/2365273004/diff/430001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): ...
4 months, 2 weeks ago (2016-10-06 21:39:29 UTC) #76
lawrencewu
Addressed asvitkine's comments. @jam, would you mind looking over the code in content/ and components/? ...
4 months, 2 weeks ago (2016-10-07 15:07:21 UTC) #78
jam (OOO)
A few questions 1) it seems gratuitous that we have a field trial for this. ...
4 months, 1 week ago (2016-10-10 15:08:41 UTC) #87
lawrencewu
On 2016/10/10 15:08:41, jam wrote: > A few questions > 1) it seems gratuitous that ...
4 months, 1 week ago (2016-10-10 18:13:48 UTC) #88
lawrencewu
On 2016/10/10 15:08:41, jam wrote: > A few questions > 1) it seems gratuitous that ...
4 months, 1 week ago (2016-10-10 18:13:51 UTC) #89
jam (OOO)
On 2016/10/10 18:13:51, lawrencewu wrote: > On 2016/10/10 15:08:41, jam wrote: > > A few ...
4 months, 1 week ago (2016-10-10 21:10:07 UTC) #90
lawrencewu
On 2016/10/10 21:10:07, jam wrote: > On 2016/10/10 18:13:51, lawrencewu wrote: > > On 2016/10/10 ...
4 months, 1 week ago (2016-10-11 13:46:28 UTC) #91
Alexei Svitkine (slow)
Just want to expand on the question about using a base::Feature: Since this affects child ...
4 months, 1 week ago (2016-10-11 15:27:35 UTC) #92
jam (OOO)
Thanks for the explanations. It's fine to land in stages then (I was previously wondering ...
4 months, 1 week ago (2016-10-11 15:40:44 UTC) #93
lawrencewu
On 2016/10/11 15:40:44, jam wrote: > Thanks for the explanations. It's fine to land in ...
4 months, 1 week ago (2016-10-11 16:13:37 UTC) #94
lawrencewu
@jam: changed it to use a bool instead of a base::Feature, mind taking a look?
4 months, 1 week ago (2016-10-12 17:31:01 UTC) #111
jam (OOO)
lgtm
4 months, 1 week ago (2016-10-12 21:18:20 UTC) #112
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2365273004/530001
4 months, 1 week ago (2016-10-13 13:48:53 UTC) #115
commit-bot: I haz the power
Committed patchset #27 (id:530001)
4 months, 1 week ago (2016-10-13 13:54:53 UTC) #117
commit-bot: I haz the power
4 months, 1 week ago (2016-10-13 13:56:38 UTC) #119
Message was sent while issue was closed.
Patchset 27 (id:??) landed as
https://crrev.com/2c93082dcc33aeaa98f7ab9f2f20ef647448372f
Cr-Commit-Position: refs/heads/master@{#425013}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd