Convert ChromeViewMsg_SetFieldTrialGroup to use mojo.

Convert ChromeViewMsg_SetFieldTrialGroup to use mojo.

BUG=577685
Review-Url: https://codereview.chromium.org/2642263002
Cr-Commit-Position: refs/heads/master@{#448132}
Committed: https://chromium.googlesource.com/chromium/src/+/f007b70562c84bb115c9178c59a507ee61a7a00d

[nigeltao1, 2017-01-20 00:26:51]

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

[commit-bot: I haz the power, 2017-01-20 00:28:02]

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

[nigeltao1, 2017-01-20 00:35:59]

nigeltao@chromium.org changed reviewers:
+ sammc@chromium.org

[nigeltao1, 2017-01-20 00:35:59]

https://codereview.chromium.org/2642263002/diff/1/chrome/browser/metrics/fiel...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/1/chrome/browser/metrics/fiel...
chrome/browser/metrics/field_trial_synchronizer.cc:48:
chrome::mojom::FieldTrialGroupSetterAssociatedPtr ftgs_interface;
I dropped some temporary printf's here, and it looks like there's 20 to 30 of
these calls during start-up, and another 2 or 3 trickle in as I browsed a few
sites. I don't think we're at the scale yet that we need to re-use a single
binding instead of continually bringing up and tearing down multiple ones, or
somehow batching the field trial group setter calls. But let me know if I should
look at keeping the binding around for longer than the method scope, or changing
the FieldTrialSynchronizer::NotifyAllRenderers API from single to batch.
Obviously, either of these is more complicated, and leads to a bigger commit.

[commit-bot: I haz the power, 2017-01-20 01:52:50]

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

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

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-20 02:35:38]

https://codereview.chromium.org/2642263002/diff/1/chrome/browser/metrics/fiel...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/1/chrome/browser/metrics/fiel...
chrome/browser/metrics/field_trial_synchronizer.cc:48:
chrome::mojom::FieldTrialGroupSetterAssociatedPtr ftgs_interface;
On 2017/01/20 00:35:59, nigeltao1 wrote:
> I dropped some temporary printf's here, and it looks like there's 20 to 30 of
> these calls during start-up, and another 2 or 3 trickle in as I browsed a few
> sites. I don't think we're at the scale yet that we need to re-use a single
> binding instead of continually bringing up and tearing down multiple ones, or
> somehow batching the field trial group setter calls. But let me know if I
should
> look at keeping the binding around for longer than the method scope, or
changing
> the FieldTrialSynchronizer::NotifyAllRenderers API from single to batch.
> Obviously, either of these is more complicated, and leads to a bigger commit.

20 to 30 notifications or 20 to 30 messages? Ideally, we would avoid slowing
down startup.

Also, no abbreviations.

[nigeltao1, 2017-01-20 02:45:22]

On 2017/01/20 02:35:38, Sam McNally wrote:
> 20 to 30 notifications or 20 to 30 messages? Ideally, we would avoid slowing
> down startup.

Maybe 20 ish notifications and 30 ish messages? It's not quite 1:1 but it's
pretty small on my set-up. Maybe it'd be worse if I was restoring a lot of tabs
on browser start-up? I'm not sure.


> Also, no abbreviations.

Done.

[Sam McNally, 2017-01-20 02:57:59]

lgtm

[nigeltao1, 2017-01-20 05:46:14]

nigeltao@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[nigeltao1, 2017-01-20 05:46:14]

R=sky,tsepez for OWNERS.

[sky, 2017-01-20 17:12:59]

https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/field_trial_synchronizer.cc:46: // channel might be NULL
in tests.
optional: NULL -> null

https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/field_trial_synchronizer.cc:50:
channel->GetRemoteAssociatedInterface(&field_trial_group_setter);
It seems weird to me to configure something via an interface that is promptly
dropped on the floor. I would expect SetFieldTrialGroup to exist on an interface
that is used for fieldtrials.

[Tom Sepez, 2017-01-20 17:42:40]

lgtm

[nigeltao1, 2017-01-20 22:40:58]

https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/field_trial_synchronizer.cc:50:
channel->GetRemoteAssociatedInterface(&field_trial_group_setter);
On 2017/01/20 17:12:59, sky wrote:
> It seems weird to me to configure something via an interface that is promptly
> dropped on the floor. I would expect SetFieldTrialGroup to exist on an
interface
> that is used for fieldtrials.

Yeah, I did say

"But let me know if I should look at keeping the binding around for longer than
the method scope, or changing the FieldTrialSynchronizer::NotifyAllRenderers API
from single to batch. Obviously, either of these is more complicated, and leads
to a bigger commit."

earlier in the code review. I'll submit this as is and investigate further.

[nigeltao1, 2017-01-20 22:41:04]

The CQ bit was checked by nigeltao@chromium.org

[commit-bot: I haz the power, 2017-01-20 22:41:31]

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

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

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

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

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

[sky, 2017-01-21 00:23:23]

I don't think you should keep the binding around, I think you should
group things that are logically related and not have an interface per
configuration option.

  -Scott

On Fri, Jan 20, 2017 at 2:40 PM,  <nigeltao@chromium.org> wrote:
>
>
https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
> File chrome/browser/metrics/field_trial_synchronizer.cc (right):
>
>
https://codereview.chromium.org/2642263002/diff/20001/chrome/browser/metrics/...
> chrome/browser/metrics/field_trial_synchronizer.cc:50:
> channel->GetRemoteAssociatedInterface(&field_trial_group_setter);
> On 2017/01/20 17:12:59, sky wrote:
>> It seems weird to me to configure something via an interface that is
> promptly
>> dropped on the floor. I would expect SetFieldTrialGroup to exist on an
> interface
>> that is used for fieldtrials.
>
> Yeah, I did say
>
> "But let me know if I should look at keeping the binding around for
> longer than the method scope, or changing the
> FieldTrialSynchronizer::NotifyAllRenderers API from single to batch.
> Obviously, either of these is more complicated, and leads to a bigger
> commit."
>
> earlier in the code review. I'll submit this as is and investigate
> further.
>
> https://codereview.chromium.org/2642263002/

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

[nigeltao1, 2017-01-21 06:10:20]

On 2017/01/21 00:23:23, sky wrote:
> I don't think you should keep the binding around, I think you should
> group things that are logically related and not have an interface per
> configuration option.

OK, I rolled it into the chrome.mojom.RendererConfiguration interface. PTAL.

[sky, 2017-01-23 16:15:04]

I'm not sure that having SetFieldTrialGroup() on an interface separate from
FieldTrialRecorder make sense. Is there someone more familiar with field trials
that could review from the perspective of how field trial usage should logically
be grouped?

[sky, 2017-01-23 16:57:48]

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

[sky, 2017-01-23 16:57:48]

+asvitkine for how to group field trial related functionality at the mojom
level.

[Alexei Svitkine (slow), 2017-01-23 21:26:26]

https://codereview.chromium.org/2642263002/diff/40001/chrome/common/field_tri...
File chrome/common/field_trial_recorder.mojom (left):

https://codereview.chromium.org/2642263002/diff/40001/chrome/common/field_tri...
chrome/common/field_trial_recorder.mojom:10: FieldTrialActivated(string
trial_name);
I think grouping things here as you had it before makes more sense.

For one, FieldTrialActivated message needs to be sent other child processes
besides renderer.

For example, PpapiHostMsg_FieldTrialActivated.

By keeping things in this file, I think we can have it shared between the two
instead of duplicating it?

[nigeltao1, 2017-02-02 06:06:04]

On 2017/01/23 16:15:04, sky wrote:
> I'm not sure that having SetFieldTrialGroup() on an interface separate from
> FieldTrialRecorder make sense. Is there someone more familiar with field
trials
> that could review from the perspective of how field trial usage should
logically
> be grouped?

Well, they have to be two separate interfaces. The SetFieldTrialGroup method is
from browser to renderer. The FieldTrialActivated method is the other way
around, from renderer (or e.g. GPU or PPAPI process) to browser. AFAICT it's not
a (synchronous) request / response RPC in the Mojo sense, but two separate but
related messages. I am not deeply familiar with field trials, though, or e.g.
what the SetFieldTrialGroup equivalent is for GPU or PPAPI. Perhaps asvitkine
can add some expertise here.

As for whether the SetFieldTrialGroup method should be in the existing
RendererConfiguration Mojo interface or a new, separate interface, in patch set
1, I originally had a new, separate interface, but rolled it into the
chrome.mojom.RendererConfiguration interface to avoid an interface per
configuration option.

[nigeltao1, 2017-02-02 06:06:30]

On 2017/01/23 21:26:26, Alexei Svitkine (very slow) wrote:
>
https://codereview.chromium.org/2642263002/diff/40001/chrome/common/field_tri...
> File chrome/common/field_trial_recorder.mojom (left):
> 
>
https://codereview.chromium.org/2642263002/diff/40001/chrome/common/field_tri...
> chrome/common/field_trial_recorder.mojom:10: FieldTrialActivated(string
> trial_name);
> I think grouping things here as you had it before makes more sense.

OK, reverted to as it was before.

[Alexei Svitkine (slow), 2017-02-02 17:29:50]

lgtm

https://codereview.chromium.org/2642263002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/field_trial_synchronizer.cc:46: // channel might be NULL
in tests.
Nit: NULL -> null

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
chrome/common/renderer_configuration.mojom:27: //	- "MarkNonSecureAs" /
"show-non-secure-passwords-cc-ui"
I'm not sure the examples are very useful here. If you do want to keep them, I
suggest just limiting it to the first couple.

[Tom Sepez, 2017-02-02 18:42:51]

lgtm

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
chrome/common/renderer_configuration.mojom:27: //	- "MarkNonSecureAs" /
"show-non-secure-passwords-cc-ui"
On 2017/02/02 17:29:50, Alexei Svitkine (very slow) wrote:
> I'm not sure the examples are very useful here. If you do want to keep them, I
> suggest just limiting it to the first couple.

Nit: Are there any rules about what's allowed (e.g punctuation, charset, hi-byte
chars, etc)?  Otherwise I wouldn't elaborate at all.

[Alexei Svitkine (slow), 2017-02-02 19:34:20]

The rules would be documented in base/metrics/field_trial.h

Perhaps we can just point to that?

On Feb 2, 2017 10:42 AM, <tsepez@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.org/2642263002/diff/60001/
> chrome/common/renderer_configuration.mojom
> File chrome/common/renderer_configuration.mojom (right):
>
> https://codereview.chromium.org/2642263002/diff/60001/
> chrome/common/renderer_configuration.mojom#newcode27
> chrome/common/renderer_configuration.mojom:27: // - "MarkNonSecureAs" /
> "show-non-secure-passwords-cc-ui"
> On 2017/02/02 17:29:50, Alexei Svitkine (very slow) wrote:
> > I'm not sure the examples are very useful here. If you do want to keep
> them, I
> > suggest just limiting it to the first couple.
>
> Nit: Are there any rules about what's allowed (e.g punctuation, charset,
> hi-byte chars, etc)? Otherwise I wouldn't elaborate at all.
>
> https://codereview.chromium.org/2642263002/
>

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

[nigeltao1, 2017-02-03 01:52:49]

sky, WDYT?

https://codereview.chromium.org/2642263002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/field_trial_synchronizer.cc (right):

https://codereview.chromium.org/2642263002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/field_trial_synchronizer.cc:46: // channel might be NULL
in tests.
On 2017/02/02 17:29:50, Alexei Svitkine (very slow) wrote:
> Nit: NULL -> null

Done.

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2642263002/diff/60001/chrome/common/renderer_...
chrome/common/renderer_configuration.mojom:27: //	- "MarkNonSecureAs" /
"show-non-secure-passwords-cc-ui"
I've replaced this with:
// See base/metrics/field_trial.h for more information.

[sky, 2017-02-03 16:42:16]

LGTM

[nigeltao1, 2017-02-04 00:03:58]

The CQ bit was checked by nigeltao@chromium.org

[nigeltao1, 2017-02-04 00:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
tsepez@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2642263002/#ps80001
(title: "Convert ChromeViewMsg_SetFieldTrialGroup to use mojo.")

[commit-bot: I haz the power, 2017-02-04 00:04:25]

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

[commit-bot: I haz the power, 2017-02-04 00:53:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486166638844910,
"parent_rev": "90396e6b910b9e4e9604074274d21ac0df14831f", "commit_rev":
"f007b70562c84bb115c9178c59a507ee61a7a00d"}

[commit-bot: I haz the power, 2017-02-04 00:54:07]

Description was changed from

==========
Convert ChromeViewMsg_SetFieldTrialGroup to use mojo.

BUG=577685
==========

to

==========
Convert ChromeViewMsg_SetFieldTrialGroup to use mojo.

BUG=577685

Review-Url: https://codereview.chromium.org/2642263002
Cr-Commit-Position: refs/heads/master@{#448132}
Committed:
https://chromium.googlesource.com/chromium/src/+/f007b70562c84bb115c9178c59a5...
==========

[commit-bot: I haz the power, 2017-02-04 00:54:08]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f007b70562c84bb115c9178c59a5...