Update Mac implemention of Time to prevent problems with...

Update Mac implemention of Time to prevent problems with
times later than the UNIX epoch 32 bit rollover in 2038 (such as
cookie expirations).  time_t is only 32 bits in MacOS X, so
we can't just use time_posix.cc

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

[pink (ping after 24hrs), 2008-11-03 18:02:50]

http://codereview.chromium.org/9249/diff/1/3
File base/time_mac.cc (right):

http://codereview.chromium.org/9249/diff/1/3#newcode44
Line 44: date.second = exploded.second +
is there anything in |exploded| about the time zone or adjustments that need to
happen for DST?

[Mark Mentovai, 2008-11-03 18:07:16]

http://codereview.chromium.org/9249/diff/1/3
File base/time_mac.cc (right):

http://codereview.chromium.org/9249/diff/1/3#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2008

http://codereview.chromium.org/9249/diff/1/3#newcode20
Line 20: // the field.  Using CFDate prevents that problem, and using
mac_absolute_time
mach_absolute_time

http://codereview.chromium.org/9249/diff/1/3#newcode29
Line 29: // offset.  The epoch is 1970-01-01 00:00:00 UTC.
"for which there is no offset" refers to no constant specifying the offset?

http://codereview.chromium.org/9249/diff/1/3#newcode42
Line 42: Time Time::FromExploded(bool is_local, const Exploded& exploded) {
Use is_local.

http://codereview.chromium.org/9249/diff/1/3#newcode55
Line 55: return Time(static_cast<int64>(seconds * kMicrosecondsPerSecond));
Too many uses of kMicrosecondsPerSecond.

http://codereview.chromium.org/9249/diff/1/3#newcode58
Line 58: void Time::Explode(bool is_local, Exploded* exploded) const {
Use is_local.

http://codereview.chromium.org/9249/diff/1/3#newcode61
Line 61: kCFAbsoluteTimeIntervalSince1970;
A continuation from a continuation doesn't need an extra four spaces of indent,
otherwise it starts looking like scoping.

http://codereview.chromium.org/9249/diff/1/3#newcode72
Line 72: kMillisecondsPerSecond;
Can you bump this over to be under the d in date?

[Amanda Walker, 2008-11-03 18:15:34]

On 2008/11/03 18:02:50, pink wrote:
> is there anything in |exploded| about the time zone or adjustments that need
to
> happen for DST?

Nope (see base/time.h).  That's denoted with is_local, which patch 2 adds
support for.

[Amanda Walker, 2008-11-03 18:20:03]

On 2008/11/03 18:07:16, Mark Mentovai wrote:
> 2008

fixed.

> mach_absolute_time

fixed.

> Line 29: // offset.  The epoch is 1970-01-01 00:00:00 UTC.
> "for which there is no offset" refers to no constant specifying the offset?

No, it refers to a zero-valued offset.  Rephrased comment.

> Use is_local.

fixed.

> Too many uses of kMicrosecondsPerSecond.

Fixed.

> Use is_local.

Fixed.

> A continuation from a continuation doesn't need an extra four spaces of
indent,
> otherwise it starts looking like scoping.

Fixed.

> Can you bump this over to be under the d in date?

No, it's not inside the cast.  Removed extra indent per earlier indenting
comment.

[Mark Mentovai, 2008-11-03 18:27:12]

http://codereview.chromium.org/9249/diff/13/213
File base/time_mac.cc (right):

http://codereview.chromium.org/9249/diff/13/213#newcode55
Line 55: is_local ? CFTimeZoneCopySystem() : NULL) +
Looks like there's a space at the end of this line that pushes it just one over
80.  Nix the space and you slide in just under the margin.

same here and below {
CFTimeZoneCopyDefault instead of CFTimeZoneCopySystem?  I don't know when they'd
differ, maybe it's possible for the former to be affected by the environment? 
It seems like that might be the case based on the docs.

You shouldn't leak the result of CFTimeZoneCopy*, whichever you use.  Use
scoped_cftyperef<CFTimeZoneRef> from base/scoped_cftyperef.h?
}

http://codereview.chromium.org/9249/diff/13/213#newcode56
Line 56: kCFAbsoluteTimeIntervalSince1970;
...but when inside a function call, you shouldn't be lining up with the call
opening.

[Amanda Walker, 2008-11-03 19:17:42]

> 80.  Nix the space and you slide in just under the margin.

obsoleted by scoped_cftyperef changes.

> CFTimeZoneCopyDefault instead of CFTimeZoneCopySystem?  I don't know when
they'd
> differ, maybe it's possible for the former to be affected by the environment? 
> It seems like that might be the case based on the docs.

The System time zone is whatever is set by the user in the pref pane.  An app
can override it (for that app only) with CFTimeZoneSetDefault (or [NSTimeZone
setDefaultTimeZone:]), but I'd argue that the user-specified time zone should be
considered "local".  

> You shouldn't leak the result of CFTimeZoneCopy*, whichever you use.  Use
> scoped_cftyperef<CFTimeZoneRef> from base/scoped_cftyperef.h?

Fixed

> http://codereview.chromium.org/9249/diff/13/213#newcode56
> Line 56: kCFAbsoluteTimeIntervalSince1970;
> ...but when inside a function call, you shouldn't be lining up with the call
> opening.

Fixed.

[Avi (use Gerrit), 2008-11-03 19:28:49]

http://codereview.chromium.org/9249/diff/13/213
File base/time_mac.cc (right):

http://codereview.chromium.org/9249/diff/13/213#newcode19
Line 19: // among other things the POSIX routines in Mac OS X fail after
2038--and
Awkward phrasing ("the among other things")

[Avi (use Gerrit), 2008-11-03 19:36:18]

lgtm