Feature to disable field trials in old versions of Chromium. Field trials...

.c	Feature to disable field trials in old versions of Chromium. Field trials
tests are (usually) monitored for a fixed length of time. With this change
field trial tests turn them selves off (will use the default group) after
the expiration time (specified in the Field Trial constructor).

BUG=13463
TEST=field_trial_unittests tests this code thorougly. spdy session and testing
     field_trials in renderer process would be very helpful. thanks much.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71820

[rtenneti, 2011-01-17 17:06:30]

Hi Jim,
  Uploaded the changes with my rtenneti@chromium.org account. Made all the
changes you had suggested in the following review. Really sorry about it (I was
using the raman.tenneti@gmail.com to upload the following).

http://codereview.chromium.org/6216004/

  Would appreciate your comments.

thanks very much,
raman

[jar (doing other things), 2011-01-18 19:39:44]

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:13: using base::TimeDelta;
Given that all the code is in a "namespace base" group, do we need these using
statements?

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:47: expiration_year_(year),
If these are only used in the constructor, and then discarded, could we avoid
having member slots to hold this data?

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:69: base::Time expiration_time =
Time::FromLocalExploded(exploded);
If we were to save anything (rather than expiration_month_, etc.), we could save
this expiration time.  I don't think we need it beyond line 70.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:208: continue;  // No definitive winner in this
trial.
This method is used to serialize fieldTrial settings for use in a renderer
processes.
Perhaps we should use the default_name when he have not finalized the group, and
yet avoid setting the browser side group to anything.  (i.e., we should not
finalize).  I'd be fearful that a process might race to spawn while a FieldTrial
is being constructed, and wouldn't want it to finalize it by accident, but the
default_group is supposed to be "safe" as a fallback.

The only risk is that an early spawn might hear about a different fieldTrial
setting than a later spawned process... but we already have the problem that
early spawns might not even know that there was a fieldTrial defined later!

WDYT?

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:219: bool FieldTrialList::StringAugmentsState(const
std::string& prior_state) {
nit: Again, my fault.  this name is poor IMO.  I had to struggle to see what the
function did :-(.

Better may be:
CreateTrialsInChildProcess(const std:::string& parent_trials)

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:247: new FieldTrial(name, kTotalProbability,
group_name, 2011, 6, 30);
That code won't define field trials in the renderer process starting later this
year.

We need to use some calculated constants such as:
kTWO_YEARS_FROM_NOW, 1, 1)

so that it is always in the future,

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:104: // doing A/B tests with timings, it is often
best to define all groups.
I wasn't clear about the meaning or intent of that comment.  What does it mean
to "not define a group?"  

Suggest removing (or rewording).

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:160: Probability divisor_;
nit: I'm probably to blame here.... but this should probably be const.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:179: // A textual name for the randomly selected
group, including the Trial name.
nit: Again, probably my fault.  I think we should delete ", including the Trial
name."

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:398: const base::FieldTrial::Probability
kSpdyCwndSynamic = 20;
nit: typo?   kSpdyCwndSynamic ==> kSpdyCwndDynamic ??

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:410: trial->AppendGroup("cwndDynamic",
kSpdyCwndSynamic);
This is already created as the default named group.  You probably don't need the
probability or this line.

[rtenneti, 2011-01-19 01:45:28]

Hi Jim,
  Sorry it took so long to publish these changes. Wanted to make sure the
changes worked ok on try bot. Made all the changes you had suggested. Added
check for "default_group_name" in CreateTrialsInChildProcess in case we are
running in single process mode.

  Would appreciate your comments.

thanks much,
raman

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:13: using base::TimeDelta;
On 2011/01/18 19:39:44, jar wrote:
> Given that all the code is in a "namespace base" group, do we need these using
> statements?

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:47: expiration_year_(year),
On 2011/01/18 19:39:44, jar wrote:
> If these are only used in the constructor, and then discarded, could we avoid
> having member slots to hold this data?

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:69: base::Time expiration_time =
Time::FromLocalExploded(exploded);
On 2011/01/18 19:39:44, jar wrote:
> If we were to save anything (rather than expiration_month_, etc.), we could
save
> this expiration time.  I don't think we need it beyond line 70.

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:208: continue;  // No definitive winner in this
trial.
On 2011/01/18 19:39:44, jar wrote:
> This method is used to serialize fieldTrial settings for use in a renderer
> processes.
> Perhaps we should use the default_name when he have not finalized the group,
and
> yet avoid setting the browser side group to anything.  (i.e., we should not
> finalize).  I'd be fearful that a process might race to spawn while a
FieldTrial
> is being constructed, and wouldn't want it to finalize it by accident, but the
> default_group is supposed to be "safe" as a fallback.
> 
> The only risk is that an early spawn might hear about a different fieldTrial
> setting than a later spawned process... but we already have the problem that
> early spawns might not even know that there was a fieldTrial defined later!
> 
> WDYT?

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:219: bool FieldTrialList::StringAugmentsState(const
std::string& prior_state) {
On 2011/01/18 19:39:44, jar wrote:
> nit: Again, my fault.  this name is poor IMO.  I had to struggle to see what
the
> function did :-(.
> 
> Better may be:
> CreateTrialsInChildProcess(const std:::string& parent_trials)

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:247: new FieldTrial(name, kTotalProbability,
group_name, 2011, 6, 30);
On 2011/01/18 19:39:44, jar wrote:
> That code won't define field trials in the renderer process starting later
this
> year.
> 
> We need to use some calculated constants such as:
> kTWO_YEARS_FROM_NOW, 1, 1)
> 
> so that it is always in the future,

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:104: // doing A/B tests with timings, it is often
best to define all groups.
On 2011/01/18 19:39:44, jar wrote:
> I wasn't clear about the meaning or intent of that comment.  What does it mean
> to "not define a group?"  
> 
> Suggest removing (or rewording).

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:160: Probability divisor_;
On 2011/01/18 19:39:44, jar wrote:
> nit: I'm probably to blame here.... but this should probably be const.

Done.

http://codereview.chromium.org/6317004/diff/47001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:179: // A textual name for the randomly selected
group, including the Trial name.
On 2011/01/18 19:39:44, jar wrote:
> nit: Again, probably my fault.  I think we should delete ", including the
Trial
> name."

Done.

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:398: const base::FieldTrial::Probability
kSpdyCwndSynamic = 20;
On 2011/01/18 19:39:44, jar wrote:
> nit: typo?   kSpdyCwndSynamic ==> kSpdyCwndDynamic ??

Done.

http://codereview.chromium.org/6317004/diff/47001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:410: trial->AppendGroup("cwndDynamic",
kSpdyCwndSynamic);
On 2011/01/18 19:39:44, jar wrote:
> This is already created as the default named group.  You probably don't need
the
> probability or this line.

Done.

[jar (doing other things), 2011-01-19 06:53:54]

LGTM

(tiny nit below)

http://codereview.chromium.org/6317004/diff/74001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6317004/diff/74001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:102: // Sometimes, based on construction
randomization, this call may causes the
nit: existing typo?  causes-->cause

[rtenneti, 2011-01-19 07:24:34]

Thanks much Jim for the thorough code review.
-raman

http://codereview.chromium.org/6317004/diff/74001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6317004/diff/74001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:102: // Sometimes, based on construction
randomization, this call may causes the
On 2011/01/19 06:53:56, jar wrote:
> nit: existing typo?  causes-->cause

Done.