Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494083005:
Replaces deprecated CFGregorianDate with CFCalendar. (Closed)
|
Created:
5 years ago by pkl (ping after 24h if needed) Modified:
4 years, 5 months ago 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. |
DescriptionReplaces 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
Messages
Total messages: 30 (11 generated)
pkl@chromium.org changed reviewers: + thakis@chromium.org
Thanks for the change! I'm afraid I have two slightly work-intensive suggestions, but they might simplify the code base a good bit. (If you do decide to take them on, I'd suggest using one CL for each.) https://codereview.chromium.org/1494083005/diff/40001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/1494083005/diff/40001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:157: static_cast<int>(exploded->tm_min), I looked at upstream NSPR to see if they have this problem, and it turns out they don't use platform-specific code for this at all. It looks like this file used the windows code above when chrome was open-sourced, and the other platforms were filled in over time. I asked wtc if he remembered why we don't just use upstream nspr code here, and he replied: """ That was done to reduce code size. It is fine to copy the original PR_ImplodeTime function to Chromium. I remember Chromium's alternative implementation has the Unix Year 2038 problem on 32-bit Linux. Copying NSPR's original code will fix that problem. """ Maybe with the three distinct implementations just using the upstream version is actually simpler? (http://lxr.mozilla.org/nspr/source/pr/src/misc/prtime.c#221) https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:168: base::ScopedCFTypeRef<CFTimeZoneRef> time_zone( I wondered why we don't just use time_posix.cc on Mac, and I found https://codereview.chromium.org/9249 -- time_t is a long on OS X and we didn't want y2k bugs. This is also explained around line 104 in this file. However, in the 7 years since then we moved to an exclusively-64-bit build on mac, where long is 64-bit. So I think we can probably delete this file and use time_posix.cc again. Maybe that's a better fix?
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
thakis: I think we should keep using time_mac.cc (because we still target 32-bit devices on iOS). WDYT? https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:168: base::ScopedCFTypeRef<CFTimeZoneRef> time_zone( On 2015/12/04 at 20:18:59, Nico wrote: > I wondered why we don't just use time_posix.cc on Mac, and I found https://codereview.chromium.org/9249 -- time_t is a long on OS X and we didn't want y2k bugs. This is also explained around line 104 in this file. However, in the 7 years since then we moved to an exclusively-64-bit build on mac, where long is 64-bit. So I think we can probably delete this file and use time_posix.cc again. Maybe that's a better fix? iOS still target 32-bit platforms so the fact that time_t is a long should probably still be relevant I think. Maybe we should continue using time_mac.cc due to that. WDYT? https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:176: CFCalendarComposeAbsoluteTime(gregorian, &absolute_time, "yMdHms", nit: you can use "yMdHmS" to also pass the "exploded.millisecond" value (the 'S' meaning 'millisecond' according to the opensource CFCalendar.c implementation http://www.opensource.apple.com/source/CF/CF-855.17/CFCalendar.c). https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:177: static_cast<int>(exploded.year), nit: exploded.year, ... are all int, so the static_cast<int>() are not required https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:211: CFCalendarDecomposeAbsoluteTime(gregorian, seconds, "yMdHms", &year, &month, nit: you can use "yMdHmsE" to also extract cf_day_of_week instead of calling "CFCalendarGetOrdinalityOfUnit" (again the 'E' can be found in http://www.opensource.apple.com/source/CF/CF-855.17/CFCalendar.c). https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:212: &day, &hour, &minute, &second); nit: you can also use &exploded->year, ... to avoid using local variables.
On 2016/01/07 at 16:36:28, sdefresne wrote: > thakis: I think we should keep using time_mac.cc (because we still target 32-bit devices on iOS). WDYT? BTW, if the answer is that yes, we want to use time_posix.cc, https://codereview.chromium.org/1569553005 does just that.
The CQ bit was checked by pkl@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
prtime.cc has been moved to a separate CL https://codereview.chromium.org/1657433002/ This CL is just for time_mac.cc. I've addressed all the comments from sdefresne as well. PTAL. Thank you! https://codereview.chromium.org/1494083005/diff/40001/base/third_party/nspr/p... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/1494083005/diff/40001/base/third_party/nspr/p... base/third_party/nspr/prtime.cc:157: static_cast<int>(exploded->tm_min), On 2015/12/04 20:18:59, Nico wrote: > I looked at upstream NSPR to see if they have this problem, and it turns out > they don't use platform-specific code for this at all. It looks like this file > used the windows code above when chrome was open-sourced, and the other > platforms were filled in over time. I asked wtc if he remembered why we don't > just use upstream nspr code here, and he replied: > > """ > That was done to reduce code size. > > It is fine to copy the original PR_ImplodeTime function to Chromium. I > remember Chromium's alternative implementation has the Unix Year 2038 > problem on 32-bit Linux. Copying NSPR's original code will fix that > problem. > """ > > Maybe with the three distinct implementations just using the upstream version is > actually simpler? (http://lxr.mozilla.org/nspr/source/pr/src/misc/prtime.c#221) This change has been moved to https://codereview.chromium.org/1657433002/ https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:168: base::ScopedCFTypeRef<CFTimeZoneRef> time_zone( On 2016/01/07 16:36:28, sdefresne wrote: > On 2015/12/04 at 20:18:59, Nico wrote: > > I wondered why we don't just use time_posix.cc on Mac, and I found > https://codereview.chromium.org/9249 -- time_t is a long on OS X and we didn't > want y2k bugs. This is also explained around line 104 in this file. However, in > the 7 years since then we moved to an exclusively-64-bit build on mac, where > long is 64-bit. So I think we can probably delete this file and use > time_posix.cc again. Maybe that's a better fix? > > iOS still target 32-bit platforms so the fact that time_t is a long should > probably still be relevant I think. Maybe we should continue using time_mac.cc > due to that. WDYT? I've taken the route of keeping time_mac.cc based on sdefresne's input above. https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:176: CFCalendarComposeAbsoluteTime(gregorian, &absolute_time, "yMdHms", On 2016/01/07 16:36:28, sdefresne (OOO till Feb 1st) wrote: > nit: you can use "yMdHmS" to also pass the "exploded.millisecond" value (the 'S' > meaning 'millisecond' according to the opensource CFCalendar.c implementation > http://www.opensource.apple.com/source/CF/CF-855.17/CFCalendar.c). Done. Also added a comment to explain 'S'. https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:177: static_cast<int>(exploded.year), On 2016/01/07 16:36:28, sdefresne (OOO till Feb 1st) wrote: > nit: exploded.year, ... are all int, so the static_cast<int>() are not required Done. https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:211: CFCalendarDecomposeAbsoluteTime(gregorian, seconds, "yMdHms", &year, &month, On 2016/01/07 16:36:28, sdefresne wrote: > nit: you can use "yMdHmsE" to also extract cf_day_of_week instead of calling > "CFCalendarGetOrdinalityOfUnit" (again the 'E' can be found in > http://www.opensource.apple.com/source/CF/CF-855.17/CFCalendar.c). Done. Added 'E'. https://codereview.chromium.org/1494083005/diff/40001/base/time/time_mac.cc#n... base/time/time_mac.cc:212: &day, &hour, &minute, &second); On 2016/01/07 16:36:28, sdefresne wrote: > nit: you can also use &exploded->year, ... to avoid using local variables. Changed to use &exploded->year etc. where appropriate.
Description was changed from ========== Replaces deprecated CFGregorianDate with CFCalendar. Several CFGregorianDate APIs have been deprecated starting in iOS8. Use CFCalendar APIs instead. BUG=565574 ========== to ========== Replaces deprecated CFGregorianDate with CFCalendar. Several CFGregorianDate APIs have been deprecated starting in iOS8. Use CFCalendar APIs instead. BUG=567983 ==========
lgtm once the obsolete comment has been removed. Nico, could you take a look too? Landing this would greatly help us in our effort of unforking Chrome on iOS. Thank you! https://codereview.chromium.org/1494083005/diff/140001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1494083005/diff/140001/base/time/time_mac.cc#... base/time/time_mac.cc:184: // Milliseconds from |exploded| is added back to |absolute_time| here Remove this comment as it is no longer valid (i.e. milliseconds are passed to CFCalendarComposeAbsoluteTime and not added to absolute time).
lgtm Please mention in the CL description that we don't use time_posix.cc because of 32-bit builds and that while mac is 64-bit everywhere now, iOS isn't yet (thanks for mentioning that sdefresne, I didn't think of that)
Description was changed from ========== Replaces deprecated CFGregorianDate with CFCalendar. Several CFGregorianDate APIs have been deprecated starting in iOS8. Use CFCalendar APIs instead. BUG=567983 ========== to ========== 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 platform on Mac or iOS, and even though Mac is only build in 64-bit, iOS still support both architectures. BUG=567983 ==========
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== 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 platform on Mac or iOS, and even though Mac is only build in 64-bit, iOS still support both architectures. BUG=567983 ========== to ========== 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 ==========
The CQ bit was checked by sdefresne@chromium.org
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
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/46687ab4cf982d41dbc029c771131fff36d0d4f1 Cr-Commit-Position: refs/heads/master@{#375438}
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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...
Message was sent while issue was closed.
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.
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_... |
