Add a mechanism to force parameters of a group to a specific value.

Add a mechanism to force parameters of a group to a specific value.

When using this, the ENTIRE parameter list of the group will be
overriden. E.g. if you override some parameters of a group, but
not others, they will be wiped out.

This is to be used in conjunction with --force-fieldtrials which
forces the group into the trial.

BUG=482720
Committed: https://crrev.com/f1c30e5421eb1cd86e31dee3a07f1f03db0dc770
Cr-Commit-Position: refs/heads/master@{#327627}

[danduong, 2015-04-21 01:50:50]

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

[danduong, 2015-04-21 01:56:15]

Just wanted to get some feedback on this.

The proposed format will require that we no longer allow trials, groups, and
params with "."

This is intended for giving perf bots an automatic mechanism to trigger
variation configurations. I don't think there will be many human users of this
command-line interface, but there should be nothing that prevents this.

[Alexei Svitkine (slow), 2015-04-22 15:42:37]

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
components/variations/variations_associated_data.h:117: //
TrialName1.GroupName1.Key1/Value1/TrialName2.GroupName2.Key2/Value2/
How about trial.group.key/value/key/value,trial.group:k/v,k/v,

that way, we don't have to repeat trial/group names if there are a lot of params
for the same group.

[Alexei Svitkine (slow), 2015-04-24 16:17:26]

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
components/variations/variations_associated_data.h:117: //
TrialName1.GroupName1.Key1/Value1/TrialName2.GroupName2.Key2/Value2/
On 2015/04/22 15:42:37, Alexei Svitkine wrote:
> How about trial.group.key/value/key/value,trial.group:k/v,k/v,
> 
> that way, we don't have to repeat trial/group names if there are a lot of
params
> for the same group.

(Oops, realise above is inconsistent, meant
trial.group.key/value/key/value,trial.group:k/v/k/v,...

[danduong, 2015-04-24 16:22:08]

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
components/variations/variations_associated_data.h:117: //
TrialName1.GroupName1.Key1/Value1/TrialName2.GroupName2.Key2/Value2/
On 2015/04/24 16:17:26, Alexei Svitkine wrote:
> On 2015/04/22 15:42:37, Alexei Svitkine wrote:
> > How about trial.group.key/value/key/value,trial.group:k/v,k/v,
> > 
> > that way, we don't have to repeat trial/group names if there are a lot of
> params
> > for the same group.
> 
> (Oops, realise above is inconsistent, meant
> trial.group.key/value/key/value,trial.group:k/v/k/v,...

I'm still confused.

do you mean:
trial1.group1:k1/v1/k2/v2,trail2.group2:k1/v1/k2/v2/k3/v3,..

[Alexei Svitkine (slow), 2015-04-24 16:31:53]

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/1/components/variations/varia...
components/variations/variations_associated_data.h:117: //
TrialName1.GroupName1.Key1/Value1/TrialName2.GroupName2.Key2/Value2/
On 2015/04/24 16:22:08, danduong wrote:
> On 2015/04/24 16:17:26, Alexei Svitkine wrote:
> > On 2015/04/22 15:42:37, Alexei Svitkine wrote:
> > > How about trial.group.key/value/key/value,trial.group:k/v,k/v,
> > > 
> > > that way, we don't have to repeat trial/group names if there are a lot of
> > params
> > > for the same group.
> > 
> > (Oops, realise above is inconsistent, meant
> > trial.group.key/value/key/value,trial.group:k/v/k/v,...
> 
> I'm still confused.
> 
> do you mean:
> trial1.group1:k1/v1/k2/v2,trail2.group2:k1/v1/k2/v2/k3/v3,..

Yes, sorry for not being very clear before.

[danduong, 2015-04-29 01:09:12]

https://codereview.chromium.org/1090813005/diff/20001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/1090813005/diff/20001/components/variations/v...
components/variations/variations_associated_data.cc:14: // #include
"net/base/escape.h"
Any suggestions on where this code should live? We have a restriction on
components/variations that prevent including net.

[Alexei Svitkine (slow), 2015-04-29 15:34:25]

https://codereview.chromium.org/1090813005/diff/20001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/1090813005/diff/20001/components/variations/v...
components/variations/variations_associated_data.cc:14: // #include
"net/base/escape.h"
On 2015/04/29 01:09:12, danduong wrote:
> Any suggestions on where this code should live? We have a restriction on
> components/variations that prevent including net.

Couple options.

One is we can put this code in chrome/

It can still call AssociateVariationParams() in the component.

Another would be to use a different escaping mechanism - since it doesn't
necessarily need the URL escaping scheme (in fact might be preferable not to use
it - not sure if the % signs in URL escaping play well with some of the shells -
i.e. on Windows %s are used for environment variable expansion).

https://codereview.chromium.org/1090813005/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:642:
command_line->GetSwitchValueASCII(switches::kForceFieldTrialParams));
Nit: Indent 2 more per style guide.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.cc:225: for (auto&
experiment_group : experiment_groups) {
Nit: const auto&

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.cc:228: if (experiment.size()
== 2) {
Nit: Reverse cond and switch to "continue;" to reduce nesting. Same below.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.h:116: // Provide a mechanism
to associate multiple set of params to multiple groups
Nit: Provide -> Provides

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.h:121: // paramenters (k1,v1
and k2,v2).
Mention something about the escape format.

You could also leave this description brief and reference the comment on the
command-line flag declaration.

https://codereview.chromium.org/1090813005/diff/80001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:540: const char
kForceFieldTrialParams[]        = "force-fieldtrial-params";
This doesn't need to be in content/.

If the code lives entirely in variations/, then it should go in
variations_switches.cc/h there (which you might need to add).

Or if we put the code in chrome/, then it would be in chrome_switches.

[danduong, 2015-04-29 21:00:09]

https://codereview.chromium.org/1090813005/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:642:
command_line->GetSwitchValueASCII(switches::kForceFieldTrialParams));
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> Nit: Indent 2 more per style guide.

Done.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
File components/variations/variations_associated_data.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.cc:225: for (auto&
experiment_group : experiment_groups) {
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.cc:228: if (experiment.size()
== 2) {
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> Nit: Reverse cond and switch to "continue;" to reduce nesting. Same below.

Done.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
File components/variations/variations_associated_data.h (right):

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.h:116: // Provide a mechanism
to associate multiple set of params to multiple groups
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> Nit: Provide -> Provides

Done.

https://codereview.chromium.org/1090813005/diff/80001/components/variations/v...
components/variations/variations_associated_data.h:121: // paramenters (k1,v1
and k2,v2).
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> Mention something about the escape format.
> 
> You could also leave this description brief and reference the comment on the
> command-line flag declaration.

Done.

https://codereview.chromium.org/1090813005/diff/80001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1090813005/diff/80001/content/public/common/c...
content/public/common/content_switches.cc:540: const char
kForceFieldTrialParams[]        = "force-fieldtrial-params";
On 2015/04/29 15:34:25, Alexei Svitkine wrote:
> This doesn't need to be in content/.
> 
> If the code lives entirely in variations/, then it should go in
> variations_switches.cc/h there (which you might need to add).
> 
> Or if we put the code in chrome/, then it would be in chrome_switches.

Done.

[Alexei Svitkine (slow), 2015-04-29 21:08:10]

Please associate a crbug with this change.

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
chrome/common/variations/variations_util.cc:52:
net::UnescapeRule::URL_SPECIAL_CHARS);
Indent 2 more. You can also run "git cl format" to let the tools do this for
you.

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
chrome/common/variations/variations_util.cc:58: return true;
You always return true. Perhaps we should return false if the param has an
invalid format and CHECK on it like we do for --force-fieldtrials=?

[danduong, 2015-04-29 21:28:14]

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
chrome/common/variations/variations_util.cc:52:
net::UnescapeRule::URL_SPECIAL_CHARS);
On 2015/04/29 21:08:10, Alexei Svitkine wrote:
> Indent 2 more. You can also run "git cl format" to let the tools do this for
> you.

Done.

https://codereview.chromium.org/1090813005/diff/90001/chrome/common/variation...
chrome/common/variations/variations_util.cc:58: return true;
On 2015/04/29 21:08:10, Alexei Svitkine wrote:
> You always return true. Perhaps we should return false if the param has an
> invalid format and CHECK on it like we do for --force-fieldtrials=?

Done.

[Alexei Svitkine (slow), 2015-04-29 22:02:39]

lgtm, please set a BUG= value for this change

https://codereview.chromium.org/1090813005/diff/110001/chrome/common/variatio...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/110001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:54: group_parts[0],
net::UnescapeRule::URL_SPECIAL_CHARS);
Nit: I think if you make a helper wrapping this net:: call the rest of the code
can be much more concise.

[danduong, 2015-04-29 22:17:49]

https://codereview.chromium.org/1090813005/diff/110001/chrome/common/variatio...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/110001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:54: group_parts[0],
net::UnescapeRule::URL_SPECIAL_CHARS);
On 2015/04/29 22:02:39, Alexei Svitkine wrote:
> Nit: I think if you make a helper wrapping this net:: call the rest of the
code
> can be much more concise.

Done.

[danduong, 2015-04-29 22:20:46]

danduong@chromium.org changed reviewers:
+ brettw@chromium.org

[Alexei Svitkine (slow), 2015-04-29 22:24:54]

still lgtm

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:24: }
Nit: add // namespace

and also an empty line above

[brettw, 2015-04-29 22:37:24]

browser_main lgtm

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:53: for (int i = 0; i <
key_values.size(); i += 2) {
int -> size_t (will give warning on Windows).

[danduong, 2015-04-29 22:40:48]

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
File chrome/common/variations/variations_util.cc (right):

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:24: }
On 2015/04/29 22:24:54, Alexei Svitkine wrote:
> Nit: add // namespace
> 
> and also an empty line above

Done.

https://codereview.chromium.org/1090813005/diff/130001/chrome/common/variatio...
chrome/common/variations/variations_util.cc:53: for (int i = 0; i <
key_values.size(); i += 2) {
On 2015/04/29 22:37:24, brettw wrote:
> int -> size_t (will give warning on Windows).

Done.

[danduong, 2015-04-29 22:42:01]

The CQ bit was checked by danduong@chromium.org

[danduong, 2015-04-29 22:42:02]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1090813005/#ps150001
(title: " ")

[commit-bot: I haz the power, 2015-04-29 22:42:54]

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

[commit-bot: I haz the power, 2015-04-29 22:58:49]

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

[commit-bot: I haz the power, 2015-04-29 22:58:50]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danduong, 2015-04-29 23:24:51]

The CQ bit was checked by danduong@chromium.org

[danduong, 2015-04-29 23:24:51]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1090813005/#ps170001
(title: " ")

[commit-bot: I haz the power, 2015-04-29 23:25:23]

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

[commit-bot: I haz the power, 2015-04-30 00:23:27]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-04-30 00:24:20]

Patchset 10 (id:??) landed as
https://crrev.com/f1c30e5421eb1cd86e31dee3a07f1f03db0dc770
Cr-Commit-Position: refs/heads/master@{#327627}