Make some field trials unforceable via command-line in the official build.

Make some field trials unforceable via command-line in the official build.

BUG=342585
TEST=Remove the OFFICIAL_BUILD ifdefs and:
1) Confirm that running with --force-fieldtrials with an unforceable experiment and a forceable one only forces the forceable one.
2) Confirm that the value which couldn't be forced in the browser also makes it unforced in the renderer.

Put the OFFICIAL_BUILD ifdefs back and confirm that unforceable experiments are still forceable in non-official builds.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250988

[gab, 2014-02-10 23:06:26]

Alexei, PTAL.

Thanks!
Gab

[Ilya Sherman, 2014-02-11 00:00:34]

Drive-by: Please file a bug for this work, and link to it from this CL.  Thanks.

[gab, 2014-02-11 00:22:53]

On 2014/02/11 00:00:34, Ilya Sherman (catching up...) wrote:
> Drive-by: Please file a bug for this work, and link to it from this CL. 
Thanks.

Done.

[Alexei Svitkine (slow), 2014-02-11 19:22:58]

Hmm, when I was thinking about how this would work originally, I was thinking
something more among the lines of:

In your code that checks the group of that experiment, it could also do its own
parse of the command-line flag to see if it was forced. And if it was forced to
a group that you don't want to allow forcing to, then the behavior you wouldn't
actually trigger the behaviour it was forced to, instead using a default
behaviour that you define.

The advantage of that is it doesn't add extra complexity to the base code
(though adds extra complexity to the feature code). I guess the other advantage
is that it still allows forcing to groups that are stricter than the default,
but I guess your implementation could be modified to do that as well (by passing
both the trial name and group name).

WDYT?

[gab, 2014-02-11 20:14:21]

On 2014/02/11 19:22:58, Alexei Svitkine wrote:
> Hmm, when I was thinking about how this would work originally, I was thinking
> something more among the lines of:
> 
> In your code that checks the group of that experiment, it could also do its
own
> parse of the command-line flag to see if it was forced. And if it was forced
to
> a group that you don't want to allow forcing to, then the behavior you
wouldn't
> actually trigger the behaviour it was forced to, instead using a default
> behaviour that you define.
> 
> The advantage of that is it doesn't add extra complexity to the base code
> (though adds extra complexity to the feature code). I guess the other
advantage
> is that it still allows forcing to groups that are stricter than the default,
> but I guess your implementation could be modified to do that as well (by
passing
> both the trial name and group name).
> 
> WDYT?

Well ideally it would default back to what it should be, not some hardcoded
default. The feature code doesn't know what its group "should be" hence why I
preferred this approach.

I also don't like feature code far from main inspecting the command-line.

[Alexei Svitkine (slow), 2014-02-11 22:44:01]

Well, the change doesn't look too unreasonable to me.

Ilya, I'm wondering what you think?

https://codereview.chromium.org/153913009/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/153913009/diff/1/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:294: typedef base::Callback<bool(const std::string&
name)>
The header for this should probably be included.

https://codereview.chromium.org/153913009/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/153913009/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:492: static const char*
KUnforceableFieldTrials[] = {
Nit: Lowercase k. Also, I don't think this needs to be static.

[gab, 2014-02-11 22:53:54]

PTAL.

Thanks!
Gab

https://codereview.chromium.org/153913009/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/153913009/diff/1/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:294: typedef base::Callback<bool(const std::string&
name)>
On 2014/02/11 22:44:02, Alexei Svitkine wrote:
> The header for this should probably be included.

Done.

https://codereview.chromium.org/153913009/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/153913009/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:492: static const char*
KUnforceableFieldTrials[] = {
On 2014/02/11 22:44:02, Alexei Svitkine wrote:
> Nit: Lowercase k. Also, I don't think this needs to be static.

Done.

Yes it needs to be static; it's a compound type (an array) and would otherwise
undergo a copy from .rodata.

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...
for details.

[Ilya Sherman, 2014-02-11 23:05:03]

LGTM % nits

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

https://codereview.chromium.org/153913009/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:492: static const char*
kUnforceableFieldTrials[] = {
nit: "const char* const"

https://codereview.chromium.org/153913009/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:495: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(KUnforceableFieldTrials); ++i) {
nit: Are you sure you need the unsafe version here?

[gab, 2014-02-12 13:48:28]

Thanks!

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

https://codereview.chromium.org/153913009/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:492: static const char*
kUnforceableFieldTrials[] = {
On 2014/02/11 23:05:04, Ilya Sherman wrote:
> nit: "const char* const"

Done.

https://codereview.chromium.org/153913009/diff/160001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:495: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(KUnforceableFieldTrials); ++i) {
On 2014/02/11 23:05:04, Ilya Sherman wrote:
> nit: Are you sure you need the unsafe version here?

Yes, arraysize() itself doesn't work for types defined in local scope:

https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=76...

ARRAYSIZE_UNSAFE is not unsafe if you are sure that the parameter is a static
array; which it clearly is here.

[gab, 2014-02-12 13:48:44]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-02-12 13:50:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/153913009/290001

[commit-bot: I haz the power, 2014-02-12 14:09:45]

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

[commit-bot: I haz the power, 2014-02-12 14:09:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Alexei Svitkine (slow), 2014-02-12 14:26:46]

lgtm

https://codereview.chromium.org/153913009/diff/290001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/153913009/diff/290001/base/metrics/field_tria...
base/metrics/field_trial.h:295: typedef base::Callback<bool(const std::string&
name)>
Nit: |name| -> |trial_name| to be consistent with how this file names this param
elsewhere.

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

https://codereview.chromium.org/153913009/diff/290001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:496: if (name ==
KUnforceableFieldTrials[i])
KUnforceableFieldTrials -> kUnforceableFieldTrials.

Please make sure this compiles in official builds when landing code with that
ifdef.

[gab, 2014-02-12 15:59:18]

Done, thanks!

https://codereview.chromium.org/153913009/diff/290001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

https://codereview.chromium.org/153913009/diff/290001/base/metrics/field_tria...
base/metrics/field_trial.h:295: typedef base::Callback<bool(const std::string&
name)>
On 2014/02/12 14:26:47, Alexei Svitkine wrote:
> Nit: |name| -> |trial_name| to be consistent with how this file names this
param
> elsewhere.

Done.

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

https://codereview.chromium.org/153913009/diff/290001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:496: if (name ==
KUnforceableFieldTrials[i])
On 2014/02/12 14:26:47, Alexei Svitkine wrote:
> KUnforceableFieldTrials -> kUnforceableFieldTrials.
> 
> Please make sure this compiles in official builds when landing code with that
> ifdef.

Right, I've already confirmed that it compiles/works locally when removing the
ifdef (see TEST section of this CL's description).

[gab, 2014-02-12 16:03:07]

@sky: OWNER for chrome/browser/chrome_browser_main.cc
@jamesr: OWNER for content/renderer/renderer_main.cc

Thanks!
Gab

[sky, 2014-02-12 18:17:28]

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

https://codereview.chromium.org/153913009/diff/490001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:629: base::Bind(&IsForceableFieldTrial));
Is there a reason you need the function and can't pass in a vector? Seems more
straightforward.

[jamesr, 2014-02-12 21:19:31]

I can't view the bug, but how are developers supposed to debug issues related to
this flag in official builds?  If the idea is that it's always forced a certain
way why not enforce that in the code and not have the command line flag at all?

[gab, 2014-02-12 23:38:36]

PTAL.

Thanks!
GAb

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

https://codereview.chromium.org/153913009/diff/490001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:629: base::Bind(&IsForceableFieldTrial));
On 2014/02/12 18:17:30, sky wrote:
> Is there a reason you need the function and can't pass in a vector? Seems more
> straightforward.

Nope, used a set instead; much easier, not sure why I didn't do that from the
get go... thanks for the suggestion!

[sky, 2014-02-13 01:04:17]

LGTM

[gab, 2014-02-13 01:05:49]

On 2014/02/13 01:04:17, sky wrote:
> LGTM

Thanks @jamesr for minor side-effect on renderer_main.cc.

Cheers!
GAb

[jamesr, 2014-02-13 01:06:29]

The bug explains the desire to have this not be controllable by command line,
but doesn't answer the debugging question at all.  I guess you just assume that
this configuation will never have bugs that manifest in official builds?  That's
some confidence.

Assuming that this situation is temporary and we'll delete the field trial as
soon as the functionality is fully launched lgtm (once you fix some formatting
up)

https://codereview.chromium.org/153913009/diff/620001/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/153913009/diff/620001/content/renderer/render...
content/renderer/renderer_main.cc:196: base::FieldTrialList::ACTIVATE_TRIALS,
std::set<std::string>());
new parameter, new line

[gab, 2014-02-13 02:49:02]

So the idea is that official builds should run in whichever mode the server says
it should run and it shouldn't be possible to alter that via command-line; if
developers want to debug an official build, they just have to run it in the mode
forced by the server.

If they want another mode they can run a non-official build in whichever mode
they please (that's exactly why this functionality is official build specific).

Does that answer your question? Otherwise I don't understand your concern...

Thanks,
Gab

https://codereview.chromium.org/153913009/diff/620001/content/renderer/render...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/153913009/diff/620001/content/renderer/render...
content/renderer/renderer_main.cc:196: base::FieldTrialList::ACTIVATE_TRIALS,
std::set<std::string>());
On 2014/02/13 01:06:30, jamesr wrote:
> new parameter, new line

Done.

[gab, 2014-02-13 02:50:47]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:51:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/153913009/760001

[commit-bot: I haz the power, 2014-02-13 14:33:54]

Change committed as 250988

[fennyfanny655, 2015-08-21 11:53:50]

fennyfanny655@gmail.com changed reviewers:
+ fennyfanny655@gmail.com