Make field trials activated in the GPU process be reflected in browser.

Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
Committed: https://crrev.com/5dc812a7851fa06da2ffd20f06c00aa553e1e19e
Cr-Commit-Position: refs/heads/master@{#398345}

[Alexei Svitkine (slow), 2016-06-02 22:16:49]

Description was changed from

==========
Make field trials activated in the GPU process be reflected in browser.

BUG=
==========

to

==========
Make field trials activated in the GPU process be reflected in browser.

BUG=616171
==========

[Alexei Svitkine (slow), 2016-06-03 14:45:07]

Patchset #1 (id:1) has been deleted

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

Description was changed from

==========
Make field trials activated in the GPU process be reflected in browser.

BUG=616171
==========

to

==========
Make field trials activated in the GPU process be reflected in browser.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

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

Patchset #1 (id:20001) has been deleted

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

asvitkine@chromium.org changed reviewers:
+ piman@chromium.org, rkaplow@chromium.org, thestig@chromium.org

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

rkaplow: overall review
piman: *gpu* changes OWNERS
thestig: chrome/renderer OWNERS

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

Description was changed from

==========
Make field trials activated in the GPU process be reflected in browser.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

to

==========
Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

[rkaplow, 2016-06-03 18:01:31]

lgtm

https://codereview.chromium.org/2034933002/diff/40001/content/common/gpu_host...
File content/common/gpu_host_messages.h (right):

https://codereview.chromium.org/2034933002/diff/40001/content/common/gpu_host...
content/common/gpu_host_messages.h:255: // Sent by the gpu process to indicate
that a fields trial has been activated.
GPU

[piman, 2016-06-03 20:44:52]

lgtm

[Alexei Svitkine (slow), 2016-06-03 20:49:05]

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

[Alexei Svitkine (slow), 2016-06-03 20:49:06]

done

Also +wfh for gpu_host_messages.h IPC review.

https://codereview.chromium.org/2034933002/diff/40001/content/common/gpu_host...
File content/common/gpu_host_messages.h (right):

https://codereview.chromium.org/2034933002/diff/40001/content/common/gpu_host...
content/common/gpu_host_messages.h:255: // Sent by the gpu process to indicate
that a fields trial has been activated.
On 2016/06/03 18:01:31, rkaplow wrote:
> GPU

Done.

[Will Harris, 2016-06-03 20:57:58]

have no security issues with the changes to content/common/gpu_host_messages.h
so lgtm

However, I couldn't find the tests for this new multi-process field-trial syncer
code - have you considered adding any (or do they already exist)?

[Alexei Svitkine (slow), 2016-06-06 15:19:49]

On 2016/06/03 20:57:58, Will Harris wrote:
> However, I couldn't find the tests for this new multi-process field-trial
syncer
> code - have you considered adding any (or do they already exist)?

That's a good point. I would very much like this to be tested, but I don't have
experience writing multi-process tests between browser and GPU process.

Ideally, I want the following:

 - Run custom test code in the browser process that instantiates a specific
trial before GPU process is launched.
 - Launch GPU process and run custom test code in that process that tests the
state of the trial.
 - Run custom test code back in browser process that verifies the trial was
activated. (Which would have to wait for receiving an IPC message...)

Do you know of any examples of a test like the above (i.e. that tests runs
custom test code both in gpu and browser process) that I could base my test off
of? Would really appreciate a pointer here.

[Alexei Svitkine (slow), 2016-06-06 20:03:21]

Still not sure how to do the more complicated multi-process test I described in
the last reply, but now I've at least added a simpler unit test for the
child_process_field_trial_syncer.cc class.

[Will Harris, 2016-06-06 20:11:31]

Sorry, I was getting around to replying to this. I don't think we have a generic
way of doing gpu process test.. the closest tests I could find that did IPC were
gpu_channel_manager_unittest.cc and gpu_channel_unittest.cc but these don't seem
to fit with what you want to test. perhaps gpu/ owners might have some advice
here.

[piman, 2016-06-06 21:17:36]

On Mon, Jun 6, 2016 at 1:11 PM, <wfh@chromium.org> wrote:

> Sorry, I was getting around to replying to this. I don't think we have a
> generic
> way of doing gpu process test.. the closest tests I could find that did
> IPC were
> gpu_channel_manager_unittest.cc and gpu_channel_unittest.cc but these
> don't seem
> to fit with what you want to test. perhaps gpu/ owners might have some
> advice
> here.
>

A browser_test is probably the easiest way to test this. When multiple
processes are involved, unit tests become difficult otherwise.


>
> https://codereview.chromium.org/2034933002/
>

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

[Alexei Svitkine (slow), 2016-06-06 21:19:33]

Yeah, I was thinking of a browser test. But is there a way to make it run
special test code in gpu process?

On Mon, Jun 6, 2016 at 5:17 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Jun 6, 2016 at 1:11 PM, <wfh@chromium.org> wrote:
>
>> Sorry, I was getting around to replying to this. I don't think we have a
>> generic
>> way of doing gpu process test.. the closest tests I could find that did
>> IPC were
>> gpu_channel_manager_unittest.cc and gpu_channel_unittest.cc but these
>> don't seem
>> to fit with what you want to test. perhaps gpu/ owners might have some
>> advice
>> here.
>>
>
> A browser_test is probably the easiest way to test this. When multiple
> processes are involved, unit tests become difficult otherwise.
>
>
>>
>> https://codereview.chromium.org/2034933002/
>>
>
>

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

[Lei Zhang, 2016-06-06 23:18:20]

chrome/renderer lgtm

https://codereview.chromium.org/2034933002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_thread_observer.h (right):

https://codereview.chromium.org/2034933002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_thread_observer.h:34: public
base::FieldTrialList::Observer {
IWYU: #include "base/metrics/field_trial.h"

[piman, 2016-06-06 23:25:11]

On Mon, Jun 6, 2016 at 2:18 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Yeah, I was thinking of a browser test. But is there a way to make it run
> special test code in gpu process?
>

You usually have to create such a way, which sometimes involves adding a
only-for-test message... not sure about the specifics in this case...

Antoine


>
> On Mon, Jun 6, 2016 at 5:17 PM, Antoine Labour <piman@chromium.org> wrote:
>
>>
>>
>> On Mon, Jun 6, 2016 at 1:11 PM, <wfh@chromium.org> wrote:
>>
>>> Sorry, I was getting around to replying to this. I don't think we have a
>>> generic
>>> way of doing gpu process test.. the closest tests I could find that did
>>> IPC were
>>> gpu_channel_manager_unittest.cc and gpu_channel_unittest.cc but these
>>> don't seem
>>> to fit with what you want to test. perhaps gpu/ owners might have some
>>> advice
>>> here.
>>>
>>
>> A browser_test is probably the easiest way to test this. When multiple
>> processes are involved, unit tests become difficult otherwise.
>>
>>
>>>
>>> https://codereview.chromium.org/2034933002/
>>>
>>
>>
>

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

[Alexei Svitkine (slow), 2016-06-07 14:48:42]

Thanks. I'll land this now and can look at writing a multiprocess browser test
in a follow up cl.

https://codereview.chromium.org/2034933002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_thread_observer.h (right):

https://codereview.chromium.org/2034933002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_thread_observer.h:34: public
base::FieldTrialList::Observer {
On 2016/06/06 23:18:20, Lei Zhang (Slow) wrote:
> IWYU: #include "base/metrics/field_trial.h"

Done.

[Alexei Svitkine (slow), 2016-06-07 14:48:51]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-06-07 14:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, piman@chromium.org, wfh@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2034933002/#ps100001
(title: "IWYU")

[commit-bot: I haz the power, 2016-06-07 14:49:18]

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

[commit-bot: I haz the power, 2016-06-07 16:03:51]

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

[commit-bot: I haz the power, 2016-06-07 16:03:53]

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

[Alexei Svitkine (slow), 2016-06-07 17:57:53]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2016-06-07 17:58:29]

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

[commit-bot: I haz the power, 2016-06-07 18:20:44]

Description was changed from

==========
Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

to

==========
Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

[commit-bot: I haz the power, 2016-06-07 18:20:45]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-07 18:23:00]

Description was changed from

==========
Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171
==========

to

==========
Make field trials activated in the GPU process be reflected in browser.

This allows field trials that are queried in the GPU process be
reflected in UMA and crash reports. Currently, they are being
omitted.

This follows https://codereview.chromium.org/2020413002 - which
refactored this functionality out of the renderer process code, so that
it could be re-used (i.e. in this CL). One modification is made to that
refactoring, which is removing the sending of the IPC message from
the syncer class, since GPU and renderer processes require different
IPC messages.

BUG=616171

Committed: https://crrev.com/5dc812a7851fa06da2ffd20f06c00aa553e1e19e
Cr-Commit-Position: refs/heads/master@{#398345}
==========

[commit-bot: I haz the power, 2016-06-07 18:23:02]

Patchset 4 (id:??) landed as
https://crrev.com/5dc812a7851fa06da2ffd20f06c00aa553e1e19e
Cr-Commit-Position: refs/heads/master@{#398345}