Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1414)

Issue 1494083005: Replaces deprecated CFGregorianDate with CFCalendar. (Closed)

Created:
3 years, 10 months ago by pkl (ping after 24h if needed)
Modified:
3 years, 3 months ago
Reviewers:
Nico, sdefresne
CC:
chromium-reviews, vmpstr+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Replaces deprecated CFGregorianDate with CFCalendar. Several CFGregorianDate APIs have been deprecated starting in iOS8. Use CFCalendar APIs instead. Keep separate implementation on Mac and iOS as base/time/time_posix.cc does not work on 32-bit architecture on Mac or iOS, and even though Mac is only build in 64-bit, iOS still support both architectures. BUG=567983 Committed: https://crrev.com/46687ab4cf982d41dbc029c771131fff36d0d4f1 Cr-Commit-Position: refs/heads/master@{#375438}

Patch Set 1 #

Patch Set 2 : added casts and comments #

Patch Set 3 : moved mac include file #

Total comments: 13

Patch Set 4 : removed change to prtime.cc #

Patch Set 5 : addressed comments #

Patch Set 6 : rebase #

Patch Set 7 : changed to int64_t #

Patch Set 8 : git cl format #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -24 lines) Patch
M base/time/time_mac.cc View 1 2 3 4 5 6 7 2 chunks +34 lines, -24 lines 1 comment Download

Messages

Total messages: 30 (11 generated)
pkl (ping after 24h if needed)
3 years, 10 months ago (2015-12-04 18:22:53 UTC) #2
Nico
Thanks for the change! I'm afraid I have two slightly work-intensive suggestions, but they might ...
3 years, 10 months ago (2015-12-04 20:18:59 UTC) #3
sdefresne
thakis: I think we should keep using time_mac.cc (because we still target 32-bit devices on ...
3 years, 9 months ago (2016-01-07 16:36:28 UTC) #5
sdefresne
On 2016/01/07 at 16:36:28, sdefresne wrote: > thakis: I think we should keep using time_mac.cc ...
3 years, 9 months ago (2016-01-07 16:54:59 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494083005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494083005/120001
3 years, 8 months ago (2016-02-03 03:15:50 UTC) #8
commit-bot: I haz the power
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even ...
3 years, 8 months ago (2016-02-03 03:15:54 UTC) #10
pkl (ping after 24h if needed)
prtime.cc has been moved to a separate CL https://codereview.chromium.org/1657433002/ This CL is just for time_mac.cc. ...
3 years, 8 months ago (2016-02-03 06:42:32 UTC) #11
sdefresne
lgtm once the obsolete comment has been removed. Nico, could you take a look too? ...
3 years, 8 months ago (2016-02-12 13:49:57 UTC) #13
Nico
lgtm Please mention in the CL description that we don't use time_posix.cc because of 32-bit ...
3 years, 8 months ago (2016-02-12 15:36:48 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494083005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494083005/140001
3 years, 8 months ago (2016-02-15 08:32:58 UTC) #17
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 8 months ago (2016-02-15 09:16:20 UTC) #19
sdefresne
lgtm
3 years, 8 months ago (2016-02-15 09:27:25 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494083005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494083005/140001
3 years, 8 months ago (2016-02-15 10:27:51 UTC) #23
commit-bot: I haz the power
Committed patchset #8 (id:140001)
3 years, 8 months ago (2016-02-15 10:34:29 UTC) #24
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/46687ab4cf982d41dbc029c771131fff36d0d4f1 Cr-Commit-Position: refs/heads/master@{#375438}
3 years, 8 months ago (2016-02-16 22:49:31 UTC) #26
maksims (do not use this acc)
If you remember, why was the reason you decided to drop milliseconds? CFCalendarComposeAbsoluteTime rounds them ...
3 years, 3 months ago (2016-07-12 11:06:49 UTC) #27
maksims (do not use this acc)
On 2016/07/12 11:06:49, maksims wrote: > If you remember, why was the reason you decided ...
3 years, 3 months ago (2016-07-12 11:08:47 UTC) #28
pkl (ping after 24h if needed)
On 2016/07/12 11:08:47, maksims wrote: > On 2016/07/12 11:06:49, maksims wrote: > > If you ...
3 years, 3 months ago (2016-07-12 17:37:56 UTC) #29
maksims (do not use this acc)
3 years, 3 months ago (2016-07-13 12:12:43 UTC) #30
Message was sent while issue was closed.
On 2016/07/12 17:37:56, pklpkl wrote:
> On 2016/07/12 11:08:47, maksims wrote:
> > On 2016/07/12 11:06:49, maksims wrote:
> > > If you remember, why was the reason you decided to drop milliseconds?
> > > CFCalendarComposeAbsoluteTime rounds them and if you check current
time_mac
> > > implementation, there is a round-trip now that indicates if the conversion
> was
> > > successful or not.
> > 
> > I dunno you might have been unaware of that fact, but FromUTC/LocalExploded
!=
> > UTCExplode or LocalExplode sometimes. 
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
> 
> My memory is a bit fuzzy. Can you point to the specific line of code to help
jog
> my memory?
> 
> This CL is intended to be functionality neutral other than getting rid of the
> use of deprecated APIs.

Yes, sure. 

If you check time_mac
(https://cs.chromium.org/chromium/src/base/time/time_mac.cc?q=time_mac.cc&sq=p...),
there is a function CFCalendarComposeAbsoluteTime() that converts exploded to
CFAbsoluteTime. The problem with this function is that it rounds time in a way
when milliseconds are not added.
When you did this patch, milliseconds were added here
https://codereview.chromium.org/1494083005/diff/60001/base/time/time_mac.cc (see
lines 185-188).

As of now, I've changed FromExploded()'s implementation (see
https://bugs.chromium.org/p/chromium/issues/detail?id=601900 and
https://cs.chromium.org/chromium/src/base/time/time_mac.cc?q=time_mac.cc&sq=p...
lines 192-208). The problem here is when conversion from exploded to local/utc
is done, it is not accurate, because converting back to exploded returns
slightly different times...sometimes. And now I am making all the callers to use
this modified method (higher layer FromUTCExploded and FromLocalExploded methods
hide this implementation now
(https://cs.chromium.org/chromium/src/base/time/time.h?sq=package:chromium&dr&...).

What I've encountered is here - https://codereview.chromium.org/2091663002/ 
To be more precise, the test that starts from line 75 fails -
https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/time_...

Powered by Google App Engine
This is Rietveld 408576698