Fix problem where field trials using kExpirationYearInFuture could get disabled.

Fix problem where field trials using kExpirationYearInFuture could get disabled.

The issue was that kExpirationYearInFuture was using system time, but the
expiration check in field_trial.cc was using build time. This CL changes
the constant to be named kExpirationYearNotExpired and sets it based on
build time.

BUG=168799
TEST=New unit test.

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

[Ilya Sherman, 2013-01-08 21:18:53]

LGTM with nits

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

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:287: // via |FactoryGetFieldTrial()|. Set to two
years from the build date.
nit: Please use two spaces after the period for consistency with the surrounding
code.

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:288: static int kExpirationYearNotExpired;
nit: I think the previous name was slightly clearer for casual users of the API

[Alexei Svitkine (slow), 2013-01-08 21:56:22]

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

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:288: static int kExpirationYearNotExpired;
On 2013/01/08 21:18:53, Ilya Sherman wrote:
> nit: I think the previous name was slightly clearer for casual users of the
API

The reason I changed the name is because after this change, the date may not
necessarily be in the future. (e.g. if it's an old build and the build date is
over two years old, then this date may be in the past). So I chose a name that
communicates exactly what it's guaranteeing (i.e. the trial won't be expired),
rather than talking about "future", which is practically not relevant to users.

Let me know if you still want me to change it (and if so to what), or if this is
okay given my explanation.

[Ilya Sherman, 2013-01-08 22:02:04]

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

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:288: static int kExpirationYearNotExpired;
On 2013/01/08 21:56:22, Alexei Svitkine wrote:
> On 2013/01/08 21:18:53, Ilya Sherman wrote:
> > nit: I think the previous name was slightly clearer for casual users of the
> API
> 
> The reason I changed the name is because after this change, the date may not
> necessarily be in the future. (e.g. if it's an old build and the build date is
> over two years old, then this date may be in the past). So I chose a name that
> communicates exactly what it's guaranteeing (i.e. the trial won't be expired),
> rather than talking about "future", which is practically not relevant to
users.
> 
> Let me know if you still want me to change it (and if so to what), or if this
is
> okay given my explanation.

I agree that the new name is more technically correct, but I think that most
clients would rather have the name be clearer in its intent, even if it might be
strictly false.  IMO kExpirationYearNotExpired is a little hard to parse.  Some
other potential names: kNoExpirationYear, kNeverExpireYear, kUnexpiredYear,
kExpirationYearNeverExpire (though this last one has a similar issue w.r.t.
being kind of hard to parse).  WDYT?

[Alexei Svitkine (slow), 2013-01-08 22:23:53]

I like kNoExpirationYear. I'll update the CL to use that tomorrow and
address your other nit and land.


On Tue, Jan 8, 2013 at 5:02 PM, <isherman@chromium.org> wrote:

>
> https://codereview.chromium.**org/11783033/diff/1/base/**
>
metrics/field_trial.h<https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h>
> File base/metrics/field_trial.h (right):
>
> https://codereview.chromium.**org/11783033/diff/1/base/**
>
metrics/field_trial.h#**newcode288<https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#newcode288>
> base/metrics/field_trial.h:**288: static int kExpirationYearNotExpired;
> On 2013/01/08 21:56:22, Alexei Svitkine wrote:
>
>> On 2013/01/08 21:18:53, Ilya Sherman wrote:
>> > nit: I think the previous name was slightly clearer for casual users
>>
> of the
>
>> API
>>
>
>  The reason I changed the name is because after this change, the date
>>
> may not
>
>> necessarily be in the future. (e.g. if it's an old build and the build
>>
> date is
>
>> over two years old, then this date may be in the past). So I chose a
>>
> name that
>
>> communicates exactly what it's guaranteeing (i.e. the trial won't be
>>
> expired),
>
>> rather than talking about "future", which is practically not relevant
>>
> to users.
>
>  Let me know if you still want me to change it (and if so to what), or
>>
> if this is
>
>> okay given my explanation.
>>
>
> I agree that the new name is more technically correct, but I think that
> most clients would rather have the name be clearer in its intent, even
> if it might be strictly false.  IMO kExpirationYearNotExpired is a
> little hard to parse.  Some other potential names: kNoExpirationYear,
> kNeverExpireYear, kUnexpiredYear, kExpirationYearNeverExpire (though
> this last one has a similar issue w.r.t. being kind of hard to parse).
> WDYT?
>
>
https://codereview.chromium.**org/11783033/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-01-09 15:24:58]

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

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:287: // via |FactoryGetFieldTrial()|. Set to two
years from the build date.
On 2013/01/08 21:18:53, Ilya Sherman wrote:
> nit: Please use two spaces after the period for consistency with the
surrounding
> code.

Done.

https://codereview.chromium.org/11783033/diff/1/base/metrics/field_trial.h#ne...
base/metrics/field_trial.h:288: static int kExpirationYearNotExpired;
Changed to kNoExpirationYear.

[commit-bot: I haz the power, 2013-01-09 15:32:16]

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

[commit-bot: I haz the power, 2013-01-09 18:15:30]

Change committed as 175831