Add base::TimeDelta::Max().

Add base::TimeDelta::Max().

TimeDelta::Max() is a maximum timedelta, larger than any timedelta you
can compare it to. The results of adding it to a time, or another time
delta are undefined.

R=ajwong@chromium.org,mkwst@chromium.org,jar@chromium.org
TBR=jamesr@chomium.org,acolwell@chromium.org,nick@chromium.org
BUG=None

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

[gavinp, 2014-02-13 17:50:07]

ajwong: PTAL.

mkwst, jar: FYI.

I'm going to add owners reviewers after one pass by ajwong, just so they don't
see unnecessary churn.

https://codereview.chromium.org/163413004/diff/1/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/163413004/diff/1/base/time/time.h#newcode64
base/time/time.h:64: static TimeDelta FromDays(int days);
This sort of scares me. What if these max values in days are passing through
implicit casts on their way back to us?

I felt it was good if ToDays and FromDays had the same types; and making ToDays
wider (i.e. int --> int64) seemed more scary than narrowing FromDays to int. My
thinking was that if somebody is casting into an int64, normally we'll preserve
numeric_limits<int>::max(), even if it's weird.

https://codereview.chromium.org/163413004/diff/1/base/time/time.h#newcode506
base/time/time.h:506: inline TimeDelta TimeDelta::FromDays(int days) {
Should these still be inlined?

[jar (doing other things), 2014-02-14 23:44:06]

I didn't mind defining a max... but you seem to be going to a lot of effort to
avoid 64 bit ints.... and I'm left wondering about the benefits here.

Can you justify?

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h#newcode64
base/time/time.h:64: static TimeDelta FromDays(int days);
Why change this?  How will this impact the ability to save/restore max?  Won't
it break?

What is the big advantage of this change?

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h#newcode85
base/time/time.h:85: static TimeDelta Max();
This seems like an effort to limit the true range of the internal 64 bit value. 
I wonder about why this is so helpful.

Are folks constantly turning things into int ranges?  Does this really help some
code a lot??

[gavinp, 2014-02-15 17:03:50]

mkwst can definitely comment here. The thing from this CL that is most important
is the Max() and is_max() members. The only reason I paid attention to the
conversions is that a review of mkwst's introduction of Time::Max() showed that
he added these conversions shortly after.

I am completely happy narrowing this CL to _only_ adding Max() and is_max(), and
splitting the conversions into a follow up CL that will especially benefit from
mkwst's wisdom.

WDYT?

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h#newcode85
base/time/time.h:85: static TimeDelta Max();
On 2014/02/14 23:44:06, jar wrote:
> This seems like an effort to limit the true range of the internal 64 bit
value. 
> I wonder about why this is so helpful.

Can you explain this better? The max value internally is the maximum 64 bit
integer.
 
> Are folks constantly turning things into int ranges?  Does this really help
some
> code a lot??

There's a bunch of call sites, as you can see in this review, where people were
rolling these on their own by assuming the internal representative was
microseconds. That's bad enough. Even worse, because overflow on signed integers
is undefined (i.e. Time and TimeDelta, which are just signed integers), math
with these "Max" elements was undefined.

The big win in this change isn't TimeDelta::Max(), it's TimeDelta::is_max(). I
have a downstream change having to do with freshness intervals that needed to
test TimeDelta::is_max(). I couldn't in good conscience write code violating the
abstraction, and that's why I wrote this CL. See
https://codereview.chromium.org/154243006/

[jar (doing other things), 2014-02-15 17:12:48]

I'm convinced.

LGTM

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/163413004/diff/440001/base/time/time.h#newcode64
base/time/time.h:64: static TimeDelta FromDays(int days);
On 2014/02/14 23:44:06, jar wrote:
> Why change this?  How will this impact the ability to save/restore max?  Won't
> it break?
> 
> What is the big advantage of this change?

I was asking: Why do you want to use days, months, and years in int form? 

Yeah... the times were stupidly long... (just as they are for even
microseconds!)... but they were needed for microseconds... and it seemed simpler
to always use the larger range.

I guess the point is that even with the int64, you still have a funky conversion
from max microseconds to days... and back.<oops> if you don't special case that
transition.

[awong, 2014-02-20 23:21:21]

LGTM

[gavinp, 2014-02-26 00:09:39]

jamesr: TBR cc/* content/*
acolwell: TBR media/*
nick: TBR sync/*

[gavinp, 2014-02-26 00:09:46]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:34:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/163413004/830001

[Paweł Hajdan Jr., 2014-02-26 05:31:56]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:41:25]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/163413004/830001

[commit-bot: I haz the power, 2014-02-26 17:57:43]

Change committed as 253502

[wolenetz, 2014-02-26 22:58:34]

On 2014/02/26 17:57:43, I haz the power (commit-bot) wrote:
> Change committed as 253502

It looks like this change regresses media layout (and, with desktop linux
proprietary_codecs enabled, media unittests):
+Infinity MediaSource durations now round-trip conversion double -> TimeDelta ->
double, hitting
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...

[wolenetz, 2014-02-26 23:25:39]

On 2014/02/26 22:58:34, wolenetz wrote:
> On 2014/02/26 17:57:43, I haz the power (commit-bot) wrote:
> > Change committed as 253502
> 
> It looks like this change regresses media layout (and, with desktop linux
> proprietary_codecs enabled, media unittests):
> +Infinity MediaSource durations now round-trip conversion double -> TimeDelta
->
> double, hitting
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...

Media layout flakiness dashboard shows new crashiness for at least:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[waffles, 2014-02-26 23:45:30]

A revert of this CL has been created in
https://codereview.chromium.org/177023003/ by waffles@chromium.org.

The reason for reverting is: The return value of kInfiniteDuration() is
sometimes (flaky) serialized to JSON. JSON cannot support non-finite numbers.

3 tests crashed:
PipelineIntegrationTest.MediaSource_ConfigChange_MP4
PipelineIntegrationTest.MediaSource_MP3
PipelineIntegrationTest.MediaSource_MP3_Icecast

[10201:10201:0226/144332:2770747412:FATAL:values.cc(200)] Check failed: false.
Non-finite (i.e. NaN or positive/negative infinity) values cannot be represented
in JSON.

[gavinp, 2014-02-27 00:04:43]

Re-opening, since this was reverted in r253643.

It seems that JSON doesn't allow serialization of doubles. Troublingly, we're
serializing the max timedelta in JSON and hoping to get it back (which is pretty
sketchy with a finite max value as well).

Oh hum.

[gavinp, 2014-02-27 00:19:07]

See https://codereview.chromium.org/177023003/ for the revert CL...

[awong, 2014-02-27 05:17:15]

Please use a different CL for the reland. It gets really confusing
otherwise.  If it gets complex, dumping some info in a bug might also be a
good idea.


On Wed, Feb 26, 2014 at 4:19 PM, <gavinp@chromium.org> wrote:

> See https://codereview.chromium.org/177023003/ for the revert CL...
>
> https://codereview.chromium.org/163413004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gavinp, 2014-02-27 14:33:48]

On 2014/02/27 05:17:15, awong wrote:
> Please use a different CL for the reland. It gets really confusing
> otherwise.  If it gets complex, dumping some info in a bug might also be a
> good idea.
> 
> 
> On Wed, Feb 26, 2014 at 4:19 PM, <mailto:gavinp@chromium.org> wrote:
> 
> > See https://codereview.chromium.org/177023003/ for the revert CL...
> >
> > https://codereview.chromium.org/163413004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

OK, I'll close this CL and land on a separate CL.

[gavinp, 2014-03-01 00:23:02]

On 2014/02/27 14:33:48, gavinp wrote:
> On 2014/02/27 05:17:15, awong wrote:
> > Please use a different CL for the reland. It gets really confusing
> > otherwise.  If it gets complex, dumping some info in a bug might also be a
> > good idea.
> > 
> > 
> > On Wed, Feb 26, 2014 at 4:19 PM, <mailto:gavinp@chromium.org> wrote:
> > 
> > > See https://codereview.chromium.org/177023003/ for the revert CL...
> > >
> > > https://codereview.chromium.org/163413004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> OK, I'll close this CL and land on a separate CL.

The reland CL is https://codereview.chromium.org/183763011/ .