Allow overriding variation parameter via chrome://flags.

Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which allows the user for a given feature to choose among predefined variations of its parameters.

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it denotes more than just instances of the Choice struct. It has been the case before; now it is even more overloaded covering also FeatureVariations. Added a few functions to make the code less dependent on how we order the options in the UI.

BUG=609059
Committed: https://crrev.com/1383d1d3c2b2696f18e2af907915fbd3da5602bd
Cr-Commit-Position: refs/heads/master@{#400416}

[jkrcal, 2016-06-06 05:53:27]

Description was changed from

==========
Allow overriding variation parameter via chrome://flags.

BUG=609059
==========

to

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters.

Minor refactoring changes:
 - Renames ConvertFlagsToSwitches to ApplyFlags to reflect its scope is wider
now - directly applying variation parameters.
 - In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

[jkrcal, 2016-06-08 14:40:19]

Description was changed from

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters.

Minor refactoring changes:
 - Renames ConvertFlagsToSwitches to ApplyFlags to reflect its scope is wider
now - directly applying variation parameters.
 - In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

to

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters. 

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE
macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the
parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first
source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

[jkrcal, 2016-06-08 16:36:42]

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

[jkrcal, 2016-06-08 16:36:43]

PTAL.

[Alexei Svitkine (slow), 2016-06-08 18:54:35]

Some initial style comments.

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags_unittest.cc:188:
result.insert(entry.choices[j].command_line_switch);
Iterating num_options times but uses the choices array gives the impression
there's a bug. Is there a reason you didn't rename choices to options?

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:723: flags_ui::PrefServiceFlagsStorage
flags_storage_(
Nit: Local vars shouldn't have their names end with _.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:137: // The name of the Finch trial in which
the selected variation parameters
Finch is an internal codename and shouldn't be used in Chromium. You can say
FieldTrial instead.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:140: const char* feature_trial;
feature_trial_name

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:258: const std::string& key =
e.NameForOption(j);
Nit: Just inline this below

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:434: }
This is a lot of nesting. Can you make the refactor code that processes a given
option (i.e. the inner block) into a helper function so that its more readable?

[jkrcal, 2016-06-09 09:55:16]

Thanks! PTAL, again.

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags_unittest.cc:188:
result.insert(entry.choices[j].command_line_switch);
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> Iterating num_options times but uses the choices array gives the impression
> there's a bug. Is there a reason you didn't rename choices to options?

I did not rename choices to options because I wanted to distinguish the more
general notion of options (applies to MULTI_VALUE, ENABLE_DISABLE_VALUE,
FEATURE_VALUE, and FEATURE_WITH_VARIATIONS_VALUE) from the notion of choice
(applies only to MULTI_VALUE).

I agree it looks like a bug. To address this, I added a getter function
ChoiceForOption(int option) and replaced all instances of "choice[i]" by
"ChoiceForOption(i)".

(The name is slightly cumbersome, yet it aligns with all other ...ForOption
functions of FeatureEntry. Another step would be to replace "Choice" by
something more specific such as "Switch". I am not sure if it is worth renaming
it all around.)

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:723: flags_ui::PrefServiceFlagsStorage
flags_storage_(
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> Nit: Local vars shouldn't have their names end with _.

Done.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:137: // The name of the Finch trial in which
the selected variation parameters
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> Finch is an internal codename and shouldn't be used in Chromium. You can say
> FieldTrial instead.

Done.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:140: const char* feature_trial;
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> feature_trial_name

Done.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:258: const std::string& key =
e.NameForOption(j);
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> Nit: Just inline this below

Done.

https://codereview.chromium.org/2036193002/diff/100001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:434: }
On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> This is a lot of nesting. Can you make the refactor code that processes a
given
> option (i.e. the inner block) into a helper function so that its more
readable?

Done. I moved it out one block deeper, I think it makes more sense so.
(the helper function is called only for the variations that are really
selected).

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

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/2036193002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags_unittest.cc:188:
result.insert(entry.choices[j].command_line_switch);
On 2016/06/09 09:55:15, jkrcal wrote:
> On 2016/06/08 18:54:35, Alexei Svitkine (slow) wrote:
> > Iterating num_options times but uses the choices array gives the impression
> > there's a bug. Is there a reason you didn't rename choices to options?
> 
> I did not rename choices to options because I wanted to distinguish the more
> general notion of options (applies to MULTI_VALUE, ENABLE_DISABLE_VALUE,
> FEATURE_VALUE, and FEATURE_WITH_VARIATIONS_VALUE) from the notion of choice
> (applies only to MULTI_VALUE).
> 
> I agree it looks like a bug. To address this, I added a getter function
> ChoiceForOption(int option) and replaced all instances of "choice[i]" by
> "ChoiceForOption(i)".
> 
> (The name is slightly cumbersome, yet it aligns with all other ...ForOption
> functions of FeatureEntry. Another step would be to replace "Choice" by
> something more specific such as "Switch". I am not sure if it is worth
renaming
> it all around.)

Adding the function sounds good to me.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
File components/flags_ui/feature_entry.cc (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:49: return
l10n_util::GetStringUTF16(IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED) +
So the syntax here is: "Enabled <description_text>". Add a comment above this to
make it clearer.

Also, I wonder if this always right. Maybe some features don't want to say
"Enabled"? Or maybe they should have a ":" after them? e.g. "Enabled: <foo>".

Anyway, I don't have a very strong opinion right now, so I think it's fine to go
with this and it can be tweaked in the future if we find there's a desire for
different syntax.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:54: description_id =
IDS_GENERIC_EXPERIMENT_CHOICE_DISABLED;
Nit: Add a comment: // |index| == |num_options - 1|

Or actually can make it a DCHECK_EQ(num_options - 1, index);

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:63: DCHECK(type ==
FeatureEntry::MULTI_VALUE);
DCHECK_EQ() when it's just one option.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:93: } else {
Nit: No } else { if the above if has a return.

(But you can keep it in the function above since that case is simpler and
doesn't require extra {}'s.)

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:52: // of the given feature [V_1, ..., V_n].
The entry will have n+2 states:
I find "a fixed list of variations of parameters of a given feature" a little
vague about the exact structure. Maybe something like "Corresponds to a
base::Feature and additional options that specify variation parameters." Then,
you can describe that each option can specify a set of variation params.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:60: enum State {
Nit: FeatureState? assuming this only applies to features and not other cases.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:90: const char* description_text;
Add a comment mentioning how it's used.

Also, explain why this doesn't need to be a description_id - that is, because
chrome://flags text should not be translated. Also mention that the other parts
use ids for historical reasons and can realistically also be moved to this.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:144: int num_options;
Nit: Keep this above the two fields above, since it applies more generally than
just to features.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:154: // Returns the choice for the option at
|index|. Only applicable for types
types -> type

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:155: // FEATURE_MULTI.
FEATURE_WITH_VARIATIONS_VALUE?

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:188: void RegisterFeatureVariationParameters(
Add a comment please.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:202: // and to select the group.
The code doesn't match the comment. "active" means something specific in
FieldTrial code - that is, the group() was queried. I don't think it makes sense
to do this here, not querying it is fine - but adjust the comment.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:206: feature_trial_name,
kTrialGroupAboutFlags);
So, this doesn't do any Feature association with the FieldTrial here.

Please mention this in the comment and document why this is fine.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:435: if
(enabled_entries.count(e.NameForOption(j)) && variation != nullptr) {
Nit: Reverse the two checks, since != nullptr check is cheaper and the other one
doesn't need to be evaluated if the first isn't true.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:55: const char kTrialGroupAboutFlags[] =
"AboutFlags";
Please put this in an "internal" namespace, since this is only in the header
file for tests.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:65: // appended via
Nit: wrapping is off.

[jkrcal, 2016-06-14 10:04:21]

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

[jkrcal, 2016-06-14 10:04:22]

Thanks Alexei! PTAL, again, if possible. 
Showing up in the MUC office sounds like a great idea :)

Would love to land it this week, before FF, if possible.
(Next week, I am London.)


jochen: PTAL at chrome/browser/*
If there is no chance you make it this week, could you please let me know to
search for another reviewer?

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
File components/flags_ui/feature_entry.cc (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:49: return
l10n_util::GetStringUTF16(IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED) +
On 2016/06/10 15:46:48, Alexei Svitkine (OOO) wrote:
> So the syntax here is: "Enabled <description_text>". Add a comment above this
to
> make it clearer.
> 
> Also, I wonder if this always right. Maybe some features don't want to say
> "Enabled"? Or maybe they should have a ":" after them? e.g. "Enabled: <foo>".
> 
> Anyway, I don't have a very strong opinion right now, so I think it's fine to
go
> with this and it can be tweaked in the future if we find there's a desire for
> different syntax.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:54: description_id =
IDS_GENERIC_EXPERIMENT_CHOICE_DISABLED;
On 2016/06/10 15:46:48, Alexei Svitkine (OOO) wrote:
> Nit: Add a comment: // |index| == |num_options - 1|
> 
> Or actually can make it a DCHECK_EQ(num_options - 1, index);

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:63: DCHECK(type ==
FeatureEntry::MULTI_VALUE);
On 2016/06/10 15:46:48, Alexei Svitkine (OOO) wrote:
> DCHECK_EQ() when it's just one option.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.cc:93: } else {
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Nit: No } else { if the above if has a return.
> 
> (But you can keep it in the function above since that case is simpler and
> doesn't require extra {}'s.)

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
File components/flags_ui/feature_entry.h (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:52: // of the given feature [V_1, ..., V_n].
The entry will have n+2 states:
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> I find "a fixed list of variations of parameters of a given feature" a little
> vague about the exact structure. Maybe something like "Corresponds to a
> base::Feature and additional options that specify variation parameters." Then,
> you can describe that each option can specify a set of variation params.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:60: enum State {
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Nit: FeatureState? assuming this only applies to features and not other cases.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:90: const char* description_text;
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Add a comment mentioning how it's used.
> 
> Also, explain why this doesn't need to be a description_id - that is, because
> chrome://flags text should not be translated. Also mention that the other
parts
> use ids for historical reasons and can realistically also be moved to this.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:144: int num_options;
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Nit: Keep this above the two fields above, since it applies more generally
than
> just to features.

I moved it even above the choices, I think it is clearer this way.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:154: // Returns the choice for the option at
|index|. Only applicable for types
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> types -> type

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fe...
components/flags_ui/feature_entry.h:155: // FEATURE_MULTI.
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> FEATURE_WITH_VARIATIONS_VALUE?

Addressed by VariationForOption(). Maybe I don't understand :)

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:188: void RegisterFeatureVariationParameters(
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Add a comment please.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:202: // and to select the group.
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> The code doesn't match the comment. "active" means something specific in
> FieldTrial code - that is, the group() was queried. I don't think it makes
sense
> to do this here, not querying it is fine - but adjust the comment.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:206: feature_trial_name,
kTrialGroupAboutFlags);
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> So, this doesn't do any Feature association with the FieldTrial here.
> 
> Please mention this in the comment and document why this is fine.

No, it does not. Beyond the scope of this CL, IMO. 
I added a TODO down to RegisterAllFeatureVariationParameters.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:435: if
(enabled_entries.count(e.NameForOption(j)) && variation != nullptr) {
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Nit: Reverse the two checks, since != nullptr check is cheaper and the other
one
> doesn't need to be evaluated if the first isn't true.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:55: const char kTrialGroupAboutFlags[] =
"AboutFlags";
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Please put this in an "internal" namespace, since this is only in the header
> file for tests.

Done.

https://codereview.chromium.org/2036193002/diff/120001/components/flags_ui/fl...
components/flags_ui/flags_state.h:65: // appended via
On 2016/06/10 15:46:49, Alexei Svitkine (OOO) wrote:
> Nit: wrapping is off.

Done.

[Alexei Svitkine (slow), 2016-06-16 09:40:57]

Generally looks good - have a few more comments below.

Have you tested how this interacts with a config coming from the server? i.e.
that it doesn't explode spectacularly ;)

To test the above, you can check instructions on how to run a local variations
server at go/chrome-variations-server and point your Chrome at that. Then, check
what happens to your local Chrome when you have both an entry here and an entry
from the server config.

https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1924: #endif
Seems like you both rebased and also addressed comments in the same patchset.
It's not the end of the world, but it makes it harder to review changes between
patchsets, so in the future it's a good idea to rebase, upload your patch set
and then address comments after that in a separate patchset.

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:209: // selected for the trial, yet. Thus,
create the trial to select the group.
Nit: Does "selected" fit on line above?

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:449: // larger changes.
Please also mention what this means in practice.

Specifically, it means that:  If the feature state is queried, it won't
automatically activate the field trial. So the related field trial will not be
reported to UMA/crash, etc.

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.h:22: namespace {
This is not quite what I meant.

There shouldn't be an anon namespace inside a header file.

Instead, do the following:

// Internal functionality exposed for tests.
namespace internal {
// your comment here
extern const char kTrialGroupAboutFlags[];
}  // namespace internal.

And then define it in the .cc file.

Also, put the above inside of namespace flags_ui.

[jkrcal, 2016-06-16 14:49:16]

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

[jkrcal, 2016-06-16 14:49:17]

Thanks, Alexei! PTAL, again.

Scott, could you PTAL at chrome/browser/*.

https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1924: #endif
On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> Seems like you both rebased and also addressed comments in the same patchset.
> It's not the end of the world, but it makes it harder to review changes
between
> patchsets, so in the future it's a good idea to rebase, upload your patch set
> and then address comments after that in a separate patchset.

You are right, I am sorry about it!

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:209: // selected for the trial, yet. Thus,
create the trial to select the group.
On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> Nit: Does "selected" fit on line above?

Done (you were right, it fits).

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.cc:449: // larger changes.
On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> Please also mention what this means in practice.
> 
> Specifically, it means that:  If the feature state is queried, it won't
> automatically activate the field trial. So the related field trial will not be
> reported to UMA/crash, etc.

Done.

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
components/flags_ui/flags_state.h:22: namespace {
On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> This is not quite what I meant.
> 
> There shouldn't be an anon namespace inside a header file.
> 
> Instead, do the following:
> 
> // Internal functionality exposed for tests.
> namespace internal {
> // your comment here
> extern const char kTrialGroupAboutFlags[];
> }  // namespace internal.
> 
> And then define it in the .cc file.
> 
> Also, put the above inside of namespace flags_ui.

Done. 
(Thanks for explaining!)

[jkrcal, 2016-06-16 14:50:27]

On 2016/06/16 14:49:17, jkrcal wrote:
> Thanks, Alexei! PTAL, again.
> 
> Scott, could you PTAL at chrome/browser/*.
> 
>
https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/2036193002/diff/140001/chrome/browser/about_f...
> chrome/browser/about_flags.cc:1924: #endif
> On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> > Seems like you both rebased and also addressed comments in the same
patchset.
> > It's not the end of the world, but it makes it harder to review changes
> between
> > patchsets, so in the future it's a good idea to rebase, upload your patch
set
> > and then address comments after that in a separate patchset.
> 
> You are right, I am sorry about it!
> 
>
https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
> File components/flags_ui/flags_state.cc (right):
> 
>
https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
> components/flags_ui/flags_state.cc:209: // selected for the trial, yet. Thus,
> create the trial to select the group.
> On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> > Nit: Does "selected" fit on line above?
> 
> Done (you were right, it fits).
> 
>
https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
> components/flags_ui/flags_state.cc:449: // larger changes.
> On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> > Please also mention what this means in practice.
> > 
> > Specifically, it means that:  If the feature state is queried, it won't
> > automatically activate the field trial. So the related field trial will not
be
> > reported to UMA/crash, etc.
> 
> Done.
> 
>
https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
> File components/flags_ui/flags_state.h (right):
> 
>
https://codereview.chromium.org/2036193002/diff/140001/components/flags_ui/fl...
> components/flags_ui/flags_state.h:22: namespace {
> On 2016/06/16 09:40:57, Alexei Svitkine (OOO) wrote:
> > This is not quite what I meant.
> > 
> > There shouldn't be an anon namespace inside a header file.
> > 
> > Instead, do the following:
> > 
> > // Internal functionality exposed for tests.
> > namespace internal {
> > // your comment here
> > extern const char kTrialGroupAboutFlags[];
> > }  // namespace internal.
> > 
> > And then define it in the .cc file.
> > 
> > Also, put the above inside of namespace flags_ui.
> 
> Done. 
> (Thanks for explaining!)

I forgot to mention - I did test how the config from server gets overriden. 
Will do further tests tomorrow.

[Alexei Svitkine (slow), 2016-06-16 15:35:04]

lgtm

[sky, 2016-06-16 17:36:23]

LGTM

[jkrcal, 2016-06-16 17:36:59]

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

[commit-bot: I haz the power, 2016-06-16 17:37:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036193002/180001

[commit-bot: I haz the power, 2016-06-16 17:41:53]

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

[commit-bot: I haz the power, 2016-06-16 17:41:55]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jkrcal, 2016-06-17 07:20:14]

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

[commit-bot: I haz the power, 2016-06-17 07:20:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036193002/220001

[commit-bot: I haz the power, 2016-06-17 07:23:22]

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

[commit-bot: I haz the power, 2016-06-17 07:23:24]

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

[jkrcal, 2016-06-17 07:32:53]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036193002/240001

[commit-bot: I haz the power, 2016-06-17 08:07:45]

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

[commit-bot: I haz the power, 2016-06-17 08:07:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jkrcal, 2016-06-17 12:10:55]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-06-17 12:10:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2036193002/#ps240001
(title: "Build deps #2")

[commit-bot: I haz the power, 2016-06-17 12:11:07]

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

[commit-bot: I haz the power, 2016-06-17 12:41:08]

Description was changed from

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters. 

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE
macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the
parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first
source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

to

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters. 

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE
macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the
parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first
source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

[commit-bot: I haz the power, 2016-06-17 12:41:10]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-17 12:42:25]

Description was changed from

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters. 

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE
macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the
parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first
source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059
==========

to

==========
Allow overriding variation parameter via chrome://flags.

Introduces a new type of FeatureEntry - FEATURE_WITH_VARIATIONS_VALUE which
allows the user for a given feature to choose among predefined variations of its
parameters.

When defining such a feature type (using the FEATURE_WITH_VARIATIONS_VALUE_TYPE
macro), one also needs to specify the trial name to which the feature belongs.
A new experiment group called "AboutFlags" is selected for the trial and the
parameters are associated with this group.

The precedence is as follows (params for a trial are associated from the first
source where they appear):
 command-line params          (--force-fieldtrial-params=...)
 about:flags                  (added by this CL)
 field trial testing config
 server config

A minor refactoring change:
In FeatureEntry, renames the notion "choice" to "option" to make it clear it
denotes more than just instances of the Choice struct. It has been the case
before; now it is even more overloaded covering also FeatureVariations. Added a
few functions to make the code less dependent on how we order the options in the
UI.

BUG=609059

Committed: https://crrev.com/1383d1d3c2b2696f18e2af907915fbd3da5602bd
Cr-Commit-Position: refs/heads/master@{#400416}
==========

[commit-bot: I haz the power, 2016-06-17 12:42:27]

Patchset 13 (id:??) landed as
https://crrev.com/1383d1d3c2b2696f18e2af907915fbd3da5602bd
Cr-Commit-Position: refs/heads/master@{#400416}