[chrome://flags] Let features override params in the same trial

[chrome://flags] Let features override params in the same trial

Previously, one could specify field trial params for a feature in
chrome://flags. However, one needed to specify some field trial name
even when reading the params only by feature name. Furthermore,
overriding field trial params for two different features that used the
same field trial name did not work.

This CL adds support for merging params for the same field trial from
several FeatureEntries. Additionally, it allows to leave the field trial
name unspecified (using a default field trial name, instead).

BUG=690450
Review-Url: https://codereview.chromium.org/2707013002
Cr-Commit-Position: refs/heads/master@{#454215}
Committed: https://chromium.googlesource.com/chromium/src/+/7a3554ebf47fa89ad775616fc99ff4b106deb584

[jkrcal, 2017-02-21 14:06:12]

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

[jkrcal, 2017-02-21 14:06:12]

Alexei, could you PTAL?

[vitaliii, 2017-02-21 15:43:27]

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:464: DCHECK(insert_result.second)
Drive-by nit:

As of now the message looks like

"Check failed: insert_result.second. multiple values for the same parameter
'variation->params[i].param_name'specified in chrome://flags!"

Please 
1) s/multiple/Multiple
2) s/name'specified/name' specified // (space is missing)
3) probably add "are" before "specified", "values ... are specified".

[jkrcal, 2017-02-24 09:38:42]

Alexei, friendly ping!

[Alexei Svitkine (slow), 2017-02-24 16:18:21]

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
File components/flags_ui/feature_entry_macros.h (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
components/flags_ui/feature_entry_macros.h:35: #define
FEATURE_WITH_VARIATIONS_VALUE_TYPE(feature, feature_variations,       \
Separately, I think we should rename FEATURE_WITH_VARIATIONS to
FEATURE_WITH_PARAMS to make things more clear.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:449: : internal::kDefaultTrialNameAboutFlags;
What happens if the user toggles multiple about:flags entries that use the
default trial name?

[jkrcal, 2017-02-27 08:04:37]

Thanks, Alexei! Answers / questions below.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
File components/flags_ui/feature_entry_macros.h (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
components/flags_ui/feature_entry_macros.h:35: #define
FEATURE_WITH_VARIATIONS_VALUE_TYPE(feature, feature_variations,       \
On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> Separately, I think we should rename FEATURE_WITH_VARIATIONS to
> FEATURE_WITH_PARAMS to make things more clear.

Makes sense. Can we postpone it to another CL and just add a TODO in this CL?

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:449: : internal::kDefaultTrialNameAboutFlags;
On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> What happens if the user toggles multiple about:flags entries that use the
> default trial name?

All the features from these multiple about:flags will get associated to this
default trial. The params from all these flags will get merged into the default
trial (with a DCHECK that there is no conflicting parameter mentioned in
multiple flags). 

Does it make sense to you?

[Alexei Svitkine (slow), 2017-02-28 18:45:35]

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
File components/flags_ui/feature_entry_macros.h (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
components/flags_ui/feature_entry_macros.h:35: #define
FEATURE_WITH_VARIATIONS_VALUE_TYPE(feature, feature_variations,       \
On 2017/02/27 08:04:36, jkrcal wrote:
> On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> > Separately, I think we should rename FEATURE_WITH_VARIATIONS to
> > FEATURE_WITH_PARAMS to make things more clear.
> 
> Makes sense. Can we postpone it to another CL and just add a TODO in this CL?

Acknowledged.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:449: : internal::kDefaultTrialNameAboutFlags;
On 2017/02/27 08:04:36, jkrcal wrote:
> On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> > What happens if the user toggles multiple about:flags entries that use the
> > default trial name?
> 
> All the features from these multiple about:flags will get associated to this
> default trial. The params from all these flags will get merged into the
default
> trial (with a DCHECK that there is no conflicting parameter mentioned in
> multiple flags). 
> 
> Does it make sense to you?

I'm not a fan of this. This could cause conflicts between parameters of
different studies.

It's nice that you're checking it here so that there's no issue if both are
specified and then conflict - so it will be caught.

However, there's still a problem where two studies have the same param names but
one of them doesn't specify it in its about flags config - e.g. relying on its
default value. Suddenly, it would now pick up the param from the other trial.

I don't think this failure mode is acceptable. I would rather a unique trial
name be generated or something like that - or just not supporting not specifying
a trial name - than introducing this failure mode.

[jkrcal, 2017-03-01 18:35:53]

Thanks! PTAL, again.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
File components/flags_ui/feature_entry_macros.h (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/feature...
components/flags_ui/feature_entry_macros.h:35: #define
FEATURE_WITH_VARIATIONS_VALUE_TYPE(feature, feature_variations,       \
On 2017/02/28 18:45:35, Alexei Svitkine (slow) wrote:
> On 2017/02/27 08:04:36, jkrcal wrote:
> > On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> > > Separately, I think we should rename FEATURE_WITH_VARIATIONS to
> > > FEATURE_WITH_PARAMS to make things more clear.
> > 
> > Makes sense. Can we postpone it to another CL and just add a TODO in this
CL?
> 
> Acknowledged.

Added a TODO.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:449: : internal::kDefaultTrialNameAboutFlags;
On 2017/02/28 18:45:35, Alexei Svitkine (slow) wrote:
> On 2017/02/27 08:04:36, jkrcal wrote:
> > On 2017/02/24 16:18:20, Alexei Svitkine (slow) wrote:
> > > What happens if the user toggles multiple about:flags entries that use the
> > > default trial name?
> > 
> > All the features from these multiple about:flags will get associated to this
> > default trial. The params from all these flags will get merged into the
> default
> > trial (with a DCHECK that there is no conflicting parameter mentioned in
> > multiple flags). 
> > 
> > Does it make sense to you?
> 
> I'm not a fan of this. This could cause conflicts between parameters of
> different studies.
> 
> It's nice that you're checking it here so that there's no issue if both are
> specified and then conflict - so it will be caught.
> 
> However, there's still a problem where two studies have the same param names
but
> one of them doesn't specify it in its about flags config - e.g. relying on its
> default value. Suddenly, it would now pick up the param from the other trial.
> 
> I don't think this failure mode is acceptable. I would rather a unique trial
> name be generated or something like that - or just not supporting not
specifying
> a trial name - than introducing this failure mode.

Fair point. Removed the support of empty trial name.

https://codereview.chromium.org/2707013002/diff/1/components/flags_ui/flags_s...
components/flags_ui/flags_state.cc:464: DCHECK(insert_result.second)
On 2017/02/21 15:43:27, vitaliii wrote:
> Drive-by nit:
> 
> As of now the message looks like
> 
> "Check failed: insert_result.second. multiple values for the same parameter
> 'variation->params[i].param_name'specified in chrome://flags!"
> 
> Please 
> 1) s/multiple/Multiple
> 2) s/name'specified/name' specified // (space is missing)
> 3) probably add "are" before "specified", "values ... are specified".

Done.

[jkrcal, 2017-03-01 18:36:04]

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

[commit-bot: I haz the power, 2017-03-01 18:36:26]

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, 2017-03-01 19:33:13]

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

[commit-bot: I haz the power, 2017-03-01 19:33:15]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-03-01 22:24:37]

lgtm

https://codereview.chromium.org/2707013002/diff/20001/components/flags_ui/fea...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2707013002/diff/20001/components/flags_ui/fea...
components/flags_ui/feature_entry.h:169: // their feature and the trial name is
thus irrelevant. This is used if type
This comment change should reverted now, right?

[jkrcal, 2017-03-02 08:13:02]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-03-02 08:13:02]

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/2707013002/#ps40001
(title: "Comments #2")

[jkrcal, 2017-03-02 08:13:13]

Thanks!

https://codereview.chromium.org/2707013002/diff/20001/components/flags_ui/fea...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2707013002/diff/20001/components/flags_ui/fea...
components/flags_ui/feature_entry.h:169: // their feature and the trial name is
thus irrelevant. This is used if type
On 2017/03/01 22:24:37, Alexei Svitkine (slow) wrote:
> This comment change should reverted now, right?

Done.

[commit-bot: I haz the power, 2017-03-02 08:13:15]

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-03-02 09:01:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488442382008400,
"parent_rev": "f6a36f921eb4b82cf76270e30ac2c74d23fedc07", "commit_rev":
"7a3554ebf47fa89ad775616fc99ff4b106deb584"}

[commit-bot: I haz the power, 2017-03-02 09:03:02]

Description was changed from

==========
[chrome://flags] Let features override params in the same trial

Previously, one could specify field trial params for a feature in
chrome://flags. However, one needed to specify some field trial name
even when reading the params only by feature name. Furthermore,
overriding field trial params for two different features that used the
same field trial name did not work.

This CL adds support for merging params for the same field trial from
several FeatureEntries. Additionally, it allows to leave the field trial
name unspecified (using a default field trial name, instead).

BUG=690450
==========

to

==========
[chrome://flags] Let features override params in the same trial

Previously, one could specify field trial params for a feature in
chrome://flags. However, one needed to specify some field trial name
even when reading the params only by feature name. Furthermore,
overriding field trial params for two different features that used the
same field trial name did not work.

This CL adds support for merging params for the same field trial from
several FeatureEntries. Additionally, it allows to leave the field trial
name unspecified (using a default field trial name, instead).

BUG=690450

Review-Url: https://codereview.chromium.org/2707013002
Cr-Commit-Position: refs/heads/master@{#454215}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a3554ebf47fa89ad775616fc99f...
==========

[commit-bot: I haz the power, 2017-03-02 09:03:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7a3554ebf47fa89ad775616fc99f...