Add simple cache backend experiment hidden behind a flag.

Add simple cache backend experiment hidden behind a command line option.

The option supports three possible values:
  --use-simple-cache-backend=on
    // Use the simple backend for all caches it supports.
  --use-simple-cache-backend=off (current default)
    // Do not use the simple cache backend.
  --use-simple-cache-backend=experiment (Android-only, will be the new default)
    // Choose dynamically based on the experiment parameters.

We are going to be using it for triggering the simple cache backend
on the current stage of development, later we will convert it to
a true experiment.

BUG=173390
TEST=Make sure simple cache backend runs with:
  chrome --use-simple-cache-backend=on
  # The actual backend triggering will be committed with:
  #   https://codereview.chromium.org/12794003

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

[SteveT, 2013-03-15 14:07:05]

Back to you with a few things and some questions.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:62: SetUpSimpleCacheFieldTrial();
We might want to wrap this in a SetupAndroidFieldTrials method which can be set
to only compile on Android.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:196: if
(parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
You want the user to turn on the switch in order to even consider rolling the
dice for the experiment?

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:203: trial->AppendGroup("Yes",
simple_cache_probability);
This sets up the trial so that everyone gets the "Yes" group. Is this what you
meant to do? If so, why?

Your CL description seems to imply that this is temporary and later you'll be
making changes to get the true flag behaviour.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.h:70: // TODO(pasko): adjust the
trial parameters and change the flag default.
nit: adjust -> Adjust

https://codereview.chromium.org/12684010/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12684010/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:1377: // Uses experiental simple cache backend
if possible.
experiental -> experimental

[pasko-google - do not use, 2013-03-15 15:36:28]

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:62: SetUpSimpleCacheFieldTrial();
On 2013/03/15 14:07:05, SteveT wrote:
> We might want to wrap this in a SetupAndroidFieldTrials method which can be
set
> to only compile on Android.

That makes sense in general. Right now we want it be available on all platforms
for ease of development. Hidden behind a flag.

Later we will switch the experiment to live on Android and probably iOS too. And
I did not find yet where the code for controlling iOS experiments should be
living.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:196: if
(parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
On 2013/03/15 14:07:05, SteveT wrote:
> You want the user to turn on the switch in order to even consider rolling the
> dice for the experiment?

Yes. This will change when we tune the experiment parameters. I outlined this in
the header file.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:203: trial->AppendGroup("Yes",
simple_cache_probability);
On 2013/03/15 14:07:05, SteveT wrote:
> This sets up the trial so that everyone gets the "Yes" group. Is this what you
> meant to do? If so, why?
> 
> Your CL description seems to imply that this is temporary and later you'll be
> making changes to get the true flag behaviour.

Yes, exactly. It is the way to enable a 100% experiment with the flag. For
development period only.

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/12684010/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.h:70: // TODO(pasko): adjust the
trial parameters and change the flag default.
On 2013/03/15 14:07:05, SteveT wrote:
> nit: adjust -> Adjust

Done.

https://codereview.chromium.org/12684010/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12684010/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:1377: // Uses experiental simple cache backend
if possible.
On 2013/03/15 14:07:05, SteveT wrote:
> experiental -> experimental

Done.

[SteveT, 2013-03-15 15:55:42]

Okay, from the changes here and our offline chats, I have a general comment:

I think this is probably fine for development, but eventually we want to get to
a state where the flag doesn't have complete control over whether or not the
experiment runs.

The convention we follow is as follows:

1) Include a flag that ENABLES the feature: Turns on the feature but does NOT
create the field trial.

2) Include a flag that DISABLES the feature: Forces off the feature but does NOT
create the field trial. (may not be necessary if this is the default state)

3) Default State: Create the field trial and use the associated percentages to
randomly turn on or off the experiment.

I *believe* that is what your comment describes, correct? (plus or minus a flag)
This is the state we want to get to if you wish you have both flags controlling
your feature, and also an experiment controlling your feature for users unaware
of the flag.


I've also noticed that the associated bug (173390) is not one that is created
from go/chrome-experiment. Have you made a bug with that template yet? We use
those bugs to track experiments in the system and ensure that the correct
approvals were made.


Please note that the M27 cut is coming soon (next Monday), so we want to move
quickly towards the state we want this experiment to ship in.

[pasko-google - do not use, 2013-03-15 17:19:13]

On 2013/03/15 15:55:42, SteveT wrote:
> Okay, from the changes here and our offline chats, I have a general comment:
> 
> I think this is probably fine for development, but eventually we want to get
to
> a state where the flag doesn't have complete control over whether or not the
> experiment runs.
> 
> The convention we follow is as follows:
> 
> 1) Include a flag that ENABLES the feature: Turns on the feature but does NOT
> create the field trial.
> 
> 2) Include a flag that DISABLES the feature: Forces off the feature but does
NOT
> create the field trial. (may not be necessary if this is the default state)
> 
> 3) Default State: Create the field trial and use the associated percentages to
> randomly turn on or off the experiment.
> 
> I *believe* that is what your comment describes, correct? (plus or minus a
flag)
> This is the state we want to get to if you wish you have both flags
controlling
> your feature, and also an experiment controlling your feature for users
unaware
> of the flag.

Slightly differently: we want the experiment be controlled by a flag. And the
Cache Backend choice be controlled by the experiment. It comes from relatively
hard constraints on usage of chrome flags in net/. Sorry for confusion here.

I uploaded the necessary flag differentiation code that is close to final. This
should be a little more code, but less confusing.

> I've also noticed that the associated bug (173390) is not one that is created
> from go/chrome-experiment. Have you made a bug with that template yet? We use
> those bugs to track experiments in the system and ensure that the correct
> approvals were made.

OK, so you track experiments from the actual code as well, cool. Very soon we
will have details on how we want to have our experiment to be made, I'll create
a bug for this, and when changing the default option value to 'experiment' I'll
attach this bug number.

With this CL we are not instantiating the experiment by default, I think it is
good.

> Please note that the M27 cut is coming soon (next Monday), so we want to move
> quickly towards the state we want this experiment to ship in.

Yes, I think we would not be able to jump in with all necessary Simple Cache
features before next Monday.

[SteveT, 2013-03-15 17:32:49]

Some comments inline.

I feel a bit like using the field trial state to pass the state of the flag to
net/ is a bit hacky. How do other net/ features get tested? Does nothing in net/
use flags?

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:199: std::string opt_value =
parsed_command_line_.GetSwitchValueASCII(
nit: const std::string

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:218: base::FieldTrial::Probability
simple_cache_probability = 1;
nit: const base::FieldTrial::Probability

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:220: trial->AppendGroup("Control",
simple_cache_probability);
For now, you're going to want to call

trial->group()

here to ensure your experiment is actually registered. Later, when you access
the field trial state (likely using FieldTrialList::FindFullName rather than
FieldTrial::group), you can remove this line.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:224: <<
switches::kUseSimpleCacheBackend << ".";
Question: There is no NOTREACHED() equivalent for Release builds?

[rvargas (doing something else), 2013-03-15 18:20:15]

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:198: if
(parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
It is much simpler to remove the command line argument and use the field trial
override (so basically there's no extra logic here).

The user would do something like
--force-fieldtrials="SimpleCacheTrial/Yes/"

to select the simple backend.

This also means that the probability of enabling the simple backend is hardcoded
to 0. (for platforms with full Finch support).

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:216: // TODO(pasko): Make this the
default on Android when the simple cache
That would also mean that there's no need to special case Android until we are
ready to run actual experiments.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:223: CHECK(0) << "Only values
(on|off|experiment) are supported for option "
Please don't crash release builds based on a command line... and avoid messages
on checks (they just increase the executable size)

[pasko-google - do not use, 2013-03-15 18:27:37]

On 2013/03/15 17:32:49, SteveT wrote:
> Some comments inline.

Thanks for quick responses!

> I feel a bit like using the field trial state to pass the state of the flag to
> net/ is a bit hacky. How do other net/ features get tested? Does nothing in
net/
> use flags?

Oh this is indeed very hacky, and I am not happy about it either. :(
The problem is, we did not find a better way to make the network stack happy
with our ways to experiment.

Command-line is not checked in net/ except some tests. The network stack team
does not want to change behaviors in net/ via command-line flags because it
would affect other projects embedding net/.

Sad.

[pasko-google - do not use, 2013-03-15 18:27:52]

Please take another look.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:199: std::string opt_value =
parsed_command_line_.GetSwitchValueASCII(
On 2013/03/15 17:32:49, SteveT wrote:
> nit: const std::string

Done.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:218: base::FieldTrial::Probability
simple_cache_probability = 1;
On 2013/03/15 17:32:49, SteveT wrote:
> nit: const base::FieldTrial::Probability

Done.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:218: base::FieldTrial::Probability
simple_cache_probability = 1;
On 2013/03/15 17:32:49, SteveT wrote:
> nit: const base::FieldTrial::Probability

Done.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:220: trial->AppendGroup("Control",
simple_cache_probability);
On 2013/03/15 17:32:49, SteveT wrote:
> For now, you're going to want to call
> 
> trial->group()
> 
> here to ensure your experiment is actually registered. Later, when you access
> the field trial state (likely using FieldTrialList::FindFullName rather than
> FieldTrial::group), you can remove this line.

Done.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:224: <<
switches::kUseSimpleCacheBackend << ".";
On 2013/03/15 17:32:49, SteveT wrote:
> Question: There is no NOTREACHED() equivalent for Release builds?

I did not find one :(

[SteveT, 2013-03-15 18:30:41]

One response to rvargas' comments.

Also, I take back what I said about next Monday being the cut. I think it's the
Monday after that (March 25).

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:198: if
(parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
This is true, but there are a couple small catches with forced:

(1) You can't control it from about:flags, so it really has to be someone
running Chrome from the command line.
(2) It still marks you as being in the experiment group, so we'll start sending
stats with your behaviour. I think this is OK because your experiment is pretty
invisible to the user, but in most cases we want to avoid people who know they
have the feature on from sending experimental stats (hence my suggestion of only
creating the FieldTrial if no flag is explicitly set).

[rvargas (doing something else), 2013-03-15 18:50:12]

> (1) You can't control it from about:flags, so it really has to be someone
> running Chrome from the command line.

Could you elaborate? I know it's not just a little hacky... it is very hacky,
but the plan was to control the behavior through the experiment because this is
not really a feature that Chrome-level code should be worrying about setting
up... in other words, the option of wiring things through all layers to select
the desired behavior from Chrome land is even worse.

So back to the question, what prevents about:flags code to set the "right"
command line option to set the field trial to the desired value?

I'm not really opposed to the command line, it's just that it means adding
redundancy and I don't like that. On the other hand, I like not adding noise to
the platforms that are not going to run the experiment :)

We can definitely live with the user bias introduced by (2).

[pasko-google - do not use, 2013-03-15 19:05:22]

On 2013/03/15 18:20:15, rvargas wrote:
>
https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
> File chrome/browser/chrome_browser_field_trials.cc (right):
> 
>
https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
> chrome/browser/chrome_browser_field_trials.cc:198: if
> (parsed_command_line_.HasSwitch(switches::kUseSimpleCacheBackend)) {
> It is much simpler to remove the command line argument and use the field trial
> override (so basically there's no extra logic here).
> 
> The user would do something like
> --force-fieldtrials="SimpleCacheTrial/Yes/"

oh that's great to know it is possible to control a field trial from command
line like this. Although as Steve notes it would not help with about:flags :(

The syntax is also a bit tricky to general audience, and I wanted more
colleagues on-board for testing the simple cache backend.

The problem of sending stats on dummy trials is slightly inconvenient. Adds
unnecessary noise from release builds, though I can possibly call
trial->Disable(), right?. Also my thinking was that developer builds do not have
experiments enables unless it is specifically asked for in settings. Is that
right?

[pasko-google - do not use, 2013-03-15 19:05:30]

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:216: // TODO(pasko): Make this the
default on Android when the simple cache
On 2013/03/15 18:20:16, rvargas wrote:
> That would also mean that there's no need to special case Android until we are
> ready to run actual experiments.

I just wanted to be clear about the intentions. I tried the simpler version in
the style of "only what we need right now", but it created legitimate
confusions. So I went closer to something that can go into release with minor
changes in probabilities.

I can remove it now since everyone seems to have a good idea of what is
happening.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:223: CHECK(0) << "Only values
(on|off|experiment) are supported for option "
On 2013/03/15 18:20:16, rvargas wrote:
> Please don't crash release builds based on a command line... and avoid
messages
> on checks (they just increase the executable size)

My argument: there is value in failing early on incorrect command-line
arguments. Otherwise people often are doing not what they think they are doing.
We have a few cases with this in net/ that killed quite a lot of my time
previously. I think the binary size cost justifies the time spent on
understanding why the code does not work as intended.

[SteveT, 2013-03-15 19:09:09]

On 2013/03/15 18:50:12, rvargas wrote:
> > (1) You can't control it from about:flags, so it really has to be someone
> > running Chrome from the command line.
> 
> Could you elaborate? I know it's not just a little hacky... it is very hacky,
> but the plan was to control the behavior through the experiment because this
is
> not really a feature that Chrome-level code should be worrying about setting
> up... in other words, the option of wiring things through all layers to select
> the desired behavior from Chrome land is even worse.
> 

So what I mean here is that if you just create the FieldTrial (i.e. don't wrap
it in any command-line checking code) and use the --force-fieldtrial flag to
change the state, it means you won't be controlling the trial state with a
switch from about:flags.

If you predicate whether or not you create a FieldTrial on a switch, that switch
can be controlled by about:flags, which exposes a UI for that flag (If that's
what you desire. Given you want "Yes"/"No"/"Experiment" as choices, it sounds
like you want to expose those as options in about:flags).

> So back to the question, what prevents about:flags code to set the "right"
> command line option to set the field trial to the desired value?
> 
> I'm not really opposed to the command line, it's just that it means adding
> redundancy and I don't like that. On the other hand, I like not adding noise
to
> the platforms that are not going to run the experiment :)
> 
> We can definitely live with the user bias introduced by (2).

[rvargas (doing something else), 2013-03-15 19:16:25]

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:223: CHECK(0) << "Only values
(on|off|experiment) are supported for option "
On 2013/03/15 19:05:30, pasko wrote:
> On 2013/03/15 18:20:16, rvargas wrote:
> > Please don't crash release builds based on a command line... and avoid
> messages
> > on checks (they just increase the executable size)
> 
> My argument: there is value in failing early on incorrect command-line
> arguments. Otherwise people often are doing not what they think they are
doing.
> We have a few cases with this in net/ that killed quite a lot of my time
> previously. I think the binary size cost justifies the time spent on
> understanding why the code does not work as intended.

Not convinced. The most common mistake is not to forget "on" or "off", but to
forget the name of the flag, and a check here doesn't help at all. This is a
pattern that we don't use, and crashing the product doesn't help random people
that want to do something with the command line. The bar for adding crash
statements is much higher.

Binary size is a real problem, and checks/dchecks don't provide an opportunity
to see the message anyway. If someone is looking for it, a comment by the check
works better.

https://codereview.chromium.org/12684010/diff/20001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/20001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:218: const
base::FieldTrial::Probability simple_cache_probability = 1;
nit: name as a constant

[gavinp, 2013-03-15 19:28:22]

On 2013/03/15 18:50:12, rvargas wrote:
> > (1) You can't control it from about:flags, so it really has to be someone
> > running Chrome from the command line.
> 
> Could you elaborate? I know it's not just a little hacky... it is very hacky,
> but the plan was to control the behavior through the experiment because this
is
> not really a feature that Chrome-level code should be worrying about setting
> up... in other words, the option of wiring things through all layers to select
> the desired behavior from Chrome land is even worse.
> 
> So back to the question, what prevents about:flags code to set the "right"
> command line option to set the field trial to the desired value?
> 
> I'm not really opposed to the command line, it's just that it means adding
> redundancy and I don't like that. On the other hand, I like not adding noise
to
> the platforms that are not going to run the experiment :)

We definitely need about:flags, so we'll need the custom command line option
rather than the standard field trial syntax. Without about:flags control, then
users experimenting with this cache in canary, or just a build they have
sideloaded onto their phone will require a rooted device to use this feature.
The android compile edit test cycle is painful enough without now requiring a
rooted device.

For the statistical bias problem, I imagine we can either live with it (probably
fine), or just have a group: FORCED_ON and SELECTED_ON, or something like that.


> 
> We can definitely live with the user bias introduced by (2).

[gavinp, 2013-03-15 19:37:25]

On 2013/03/15 15:55:42, SteveT wrote:
> Okay, from the changes here and our offline chats, I have a general comment:
> 
> I think this is probably fine for development, but eventually we want to get
to
> a state where the flag doesn't have complete control over whether or not the
> experiment runs.
> 
> The convention we follow is as follows:
> 
> 1) Include a flag that ENABLES the feature: Turns on the feature but does NOT
> create the field trial.
> 
> 2) Include a flag that DISABLES the feature: Forces off the feature but does
NOT
> create the field trial. (may not be necessary if this is the default state)
> 
> 3) Default State: Create the field trial and use the associated percentages to
> randomly turn on or off the experiment.
> 
> I *believe* that is what your comment describes, correct? (plus or minus a
flag)
> This is the state we want to get to if you wish you have both flags
controlling
> your feature, and also an experiment controlling your feature for users
unaware
> of the flag.

To amplify my earlier comment: can we please just set this feature up the
standard way that SteveT describes? I don't know what's redundant about doing
things the way people expect. Yes, I know we'll end up parsing command line
options in net/.

> 
> 
> I've also noticed that the associated bug (173390) is not one that is created
> from go/chrome-experiment. Have you made a bug with that template yet? We use
> those bugs to track experiments in the system and ensure that the correct
> approvals were made.
> 
> 
> Please note that the M27 cut is coming soon (next Monday), so we want to move
> quickly towards the state we want this experiment to ship in.

[rvargas (doing something else), 2013-03-15 21:19:37]

Disclaimer: I'm not pushing to eliminate the command line (anymore)... not
having noise on other platforms outweighs the benefits. What I asked before in
this thread was more of a question that I consider interesting and not an
argument for changing this CL.

In other words, my only issue with Patch Set 4 is the CHECKS.

Going back to the abstract discussion about what's possible and what not:

> So what I mean here is that if you just create the FieldTrial (i.e. don't wrap
> it in any command-line checking code) and use the --force-fieldtrial flag to
> change the state, it means you won't be controlling the trial state with a
> switch from about:flags.

Yes I understood what you meant, but I don't see why it is not possible to
control a --force-fieldtrial "command line flag" from about:flags

> We definitely need about:flags, so we'll need the custom command line option
> rather than the standard field trial syntax. Without about:flags control, then

You are missing my point. We all agree that we need about:flags, and that was
also clear at yesterday's meeting. That doesn't mean that the only way to
implement the user controllable option is to add an extra command line.

> To amplify my earlier comment: can we please just set this feature up the
> standard way that SteveT describes? I don't know what's redundant about doing
> things the way people expect. Yes, I know we'll end up parsing command line
> options in net/.

Having two separate command line options that control the same thing is
redundant. Why we have that many command line args? because all of them are not
experiments, and the option of override an experiment with a command line is
relatively new.

------

command line parsing in net?. Please no. A command line is not the way to
control the behavior of a library.

[pasko-google - do not use, 2013-03-15 21:30:57]

Steve, please take another look, made a few changes in CHECKs.

I'm just watching the ball thrown from one side to another on where to have the
flag, and getting a little sleepy/bored. I do not have an opinion on this, both
ways sound almost equally hacky. The functionality being in place is all I dream
of.

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/12684010/diff/12001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:223: CHECK(0) << "Only values
(on|off|experiment) are supported for option "
On 2013/03/15 19:16:25, rvargas wrote:
> On 2013/03/15 19:05:30, pasko wrote:
> > On 2013/03/15 18:20:16, rvargas wrote:
> > > Please don't crash release builds based on a command line... and avoid
> > messages
> > > on checks (they just increase the executable size)
> > 
> > My argument: there is value in failing early on incorrect command-line
> > arguments. Otherwise people often are doing not what they think they are
> doing.
> > We have a few cases with this in net/ that killed quite a lot of my time
> > previously. I think the binary size cost justifies the time spent on
> > understanding why the code does not work as intended.
> 
> Not convinced. The most common mistake is not to forget "on" or "off", but to
> forget the name of the flag, and a check here doesn't help at all. This is a
> pattern that we don't use, and crashing the product doesn't help random people
> that want to do something with the command line. The bar for adding crash
> statements is much higher.
> 
> Binary size is a real problem, and checks/dchecks don't provide an opportunity
> to see the message anyway. If someone is looking for it, a comment by the
check
> works better.

I still have the opinion that the message overhead is small compared to the
overall experiment overhead. Though this is all minor, don't want to spend too
much time on it.

Done.

[gavinp, 2013-03-15 22:08:11]

On 2013/03/15 21:19:37, rvargas wrote:
> Disclaimer: I'm not pushing to eliminate the command line (anymore)... not
> having noise on other platforms outweighs the benefits. What I asked before in
> this thread was more of a question that I consider interesting and not an
> argument for changing this CL.
> 
> In other words, my only issue with Patch Set 4 is the CHECKS.
> 
> Going back to the abstract discussion about what's possible and what not:
> 
> > So what I mean here is that if you just create the FieldTrial (i.e. don't
wrap
> > it in any command-line checking code) and use the --force-fieldtrial flag to
> > change the state, it means you won't be controlling the trial state with a
> > switch from about:flags.
> 
> Yes I understood what you meant, but I don't see why it is not possible to
> control a --force-fieldtrial "command line flag" from about:flags
> 

If you've investigated this, and that's true, that's awesome. Thanks. I am not a
huge fan of having the make believe field trial for forcing it, but if that's
what lets this code get in, that's great. We need to progress.

> > We definitely need about:flags, so we'll need the custom command line option
> > rather than the standard field trial syntax. Without about:flags control,
then
> 
> You are missing my point. We all agree that we need about:flags, and that was
> also clear at yesterday's meeting. That doesn't mean that the only way to
> implement the user controllable option is to add an extra command line.
> 
> > To amplify my earlier comment: can we please just set this feature up the
> > standard way that SteveT describes? I don't know what's redundant about
doing
> > things the way people expect. Yes, I know we'll end up parsing command line
> > options in net/.
> 
> Having two separate command line options that control the same thing is
> redundant. Why we have that many command line args? because all of them are
not
> experiments, and the option of override an experiment with a command line is
> relatively new.
> 
> ------
> 
> command line parsing in net?. Please no. A command line is not the way to
> control the behavior of a library.

[rvargas (doing something else), 2013-03-15 22:30:25]

lgtm

[SteveT, 2013-03-16 18:22:09]

lgtm - thanks!

Can you include some TEST= instructions, please?

[pasko-google - do not use, 2013-03-17 05:26:38]

On 2013/03/16 18:22:09, SteveT wrote:
> lgtm - thanks!
> 
> Can you include some TEST= instructions, please?

Done. Thanks! It will be possible to test soon.

[commit-bot: I haz the power, 2013-03-17 05:48:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pasko@google.com/12684010/33001

[commit-bot: I haz the power, 2013-03-17 10:29:22]

Change committed as 188634