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

[ramant (doing other things), 2011-01-12 03:22:23]

Hi Jim,
  This is same patch. Added the DCHECK for all_remaining_probability_set_ when
group_name is accessed.

  http://codereview.chromium.org/6213001/

thanks much,
raman

[rtenneti, 2011-01-14 05:44:32]

Hi Jim,
  Implemented the changes we had discussed. Specifying the default group and
expiration time in the constructor. Would appreciate your comments.

Thanks very much,
raman

[rtenneti, 2011-01-14 19:07:51]

Hi Jim,
  Fixed a small bug in websocket_experiment_runner.cc (and small nit changes to
comments).

thanks much,
raman

[rvargas (doing something else), 2011-01-15 00:16:55]

Drive by.

http://codereview.chromium.org/6216004/diff/32001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6216004/diff/32001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:178: // After June 30, 2011 builds, it will
always be in defaut group.
nit: Remove the comment, as it doesn't make any sense here.

[rtenneti, 2011-01-15 00:28:01]

Thanks very much. Also tried to make comments read better.

-raman

[rtenneti, 2011-01-15 00:29:39]

Hi Jim,
  Cleaned up the comments in field_trial.h. Would appreciate your comments.

thanks much,
raman

http://codereview.chromium.org/6216004/diff/32001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6216004/diff/32001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:178: // After June 30, 2011 builds, it will
always be in defaut group.
On 2011/01/15 00:16:55, rvargas wrote:
> nit: Remove the comment, as it doesn't make any sense here.

Done.

[jar (doing other things), 2011-01-15 20:52:47]

Code is looking pretty good. Several comments below.

Thanks,

Jim

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

http://codereview.chromium.org/6216004/diff/69001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:121: static Time integral_build_time;
Don't bother caching this.  There may be a race on multiple threads to
initialize this static. The perf cost of not caching is insignificant, so it is
hard to justify violation of style guide mandate that only POD be contained in
statics.  You could probably get away with the race, as the instance is mostly a
plain structure with no virtual methods, so premature (uninitialized) use
wouldn't be very tragic (I don't think it could crash, and could only give a
wrong answer to a racing thread), but it seems unnecessary to think about.

http://codereview.chromium.org/6216004/diff/69001/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:245: new FieldTrial(name, kTotalProbability,
group_name, 2099, 12, 31);
This constant is a little dangerous on two accounts. First, the code won't "work
as planned" after 2099, but even more problematic, is that the data conversion
may wrap on some platforms!  For instance, this is beyond 2038, when the "unix
millenium bug" is scheduled, so it probably wraps on linux."

I'd suggest passing in a legal date (yesterday? last year? 2010? using manifest
constants), and allowing the code to default to using the default group name,
which is exactly what you wanted anyway ;-).

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

http://codereview.chromium.org/6216004/diff/69001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:112: // that histograms will get unique names via the
MakeName() methods.
Comment looks great up to the last sentence. 

The last sentence about "defin all groupss" should be moved to the AddGroup()
method.  I think you mean to say something about each group name should be
unique, rather than define all groups, as every group is "defined" when you add
it.  Perhaps you wanted to say the group name should not be an empty string...
and that might better be enforced as a DCHECK.

http://codereview.chromium.org/6216004/diff/69001/base/metrics/field_trial_un...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6216004/diff/69001/base/metrics/field_trial_un...
base/metrics/field_trial_unittest.cc:31: new FieldTrial(name1, 10, "default name
1 test", 2011, 12, 31);
For these constants, I'd suggest writing a little function to give you dates
that are in the future, or in the past, as needed.

You could put the code to create these in the constructor for FieldTrialTest
class, and then store the constants in member variables for use throughout the
tests in this file.

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

http://codereview.chromium.org/6216004/diff/69001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:234: // After June 30, 2011 builds, it will
always be in defaut group.
nit: defaut-->default

http://codereview.chromium.org/6216004/diff/69001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:413: trial->group();
The API should not require users to call this just for side effects.  This
should happen when the caller asks because they need to know, or when a
MakeName() call takes place.

http://codereview.chromium.org/6216004/diff/69001/chrome/browser/net/websocke...
File chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc
(right):

http://codereview.chromium.org/6216004/diff/69001/chrome/browser/net/websocke...
chrome/browser/net/websocket_experiment/websocket_experiment_runner.cc:34: 30));
nit: indent to match other arguments after paren.

http://codereview.chromium.org/6216004/diff/69001/chrome/renderer/render_thre...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/6216004/diff/69001/chrome/renderer/render_thre...
chrome/renderer/render_thread.cc:311: "default_prelaunch_gpu_process", 2011, 6,
30));
nit: better may be to add "_default" as a postfix instead of a prexfix, so that
if there is a new histogram, it will alphabetize next to the non-default case.

Better yet may be to speak with GPU folks and see why we don't have a second
group name.  I suspect this was a bug.

http://codereview.chromium.org/6216004/diff/69001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6216004/diff/69001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:180: trial1->group();
nit: API should not require users to explicitly call group() to finalize.