Make it so disabled field trials are not reported as active.

Make it so disabled field trials are not reported as active.

BUG=160310
TEST=New FieldTrialTest.DisabledTrialNotActive test and updated existing tests.

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

[Ilya Sherman, 2012-11-09 22:45:58]

LGTM

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

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial.c...
base/metrics/field_trial.cc:148: if (enable_field_trial_)
optional nit: Maybe combine this with the if-stmt above?

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial_u...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial_u...
base/metrics/field_trial_unittest.cc:392:
ASSERT_FALSE(FieldTrialList::TrialExists("xxx"));
Just curious, why did you change these from EXPECTs to ASSERTs?

[Alexei Svitkine (slow), 2012-11-09 23:42:39]

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

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial.c...
base/metrics/field_trial.cc:148: if (enable_field_trial_)
On 2012/11/09 22:45:59, Ilya Sherman wrote:
> optional nit: Maybe combine this with the if-stmt above?

I thought of that, but then |group_reported_| wouldn't accurately track whether
group() was called externally (which wouldn't matter for the current use of that
variable, but may be useful to know in the future).

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial_u...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/11359136/diff/14002/base/metrics/field_trial_u...
base/metrics/field_trial_unittest.cc:392:
ASSERT_FALSE(FieldTrialList::TrialExists("xxx"));
On 2012/11/09 22:45:59, Ilya Sherman wrote:
> Just curious, why did you change these from EXPECTs to ASSERTs?

It's a precondition, if that line fails, then the rest of the test results
aren't valid, so the test might as well not bother running the rest and exit
early.

[commit-bot: I haz the power, 2012-11-09 23:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11359136/14002

[commit-bot: I haz the power, 2012-11-10 03:19:45]

Change committed as 167045