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

Issue 1988663002: Add: check exploded time is properly converted (Closed)

Created:
3 years, 5 months ago by maksims (do not use this acc)
Modified:
2 years, 11 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add: check exploded time is properly converted. This cl introduces time checking in posix-like and mac systems. base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail without returning a proper error. This fix does the following: 1) After calculations are done, create UTC or local time. 2) Convert UTC or local time back to exploded 3) Compare original exploded with converted one 4) If times are not equal, then return Time(0) indicating an error. Windows implementation already returns Time(0) on error. BUG=601900 Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Committed: https://crrev.com/558f168054566ecbc611918093398add04dc13a4 Cr-Original-Commit-Position: refs/heads/master@{#396638} Cr-Commit-Position: refs/heads/master@{#399794}

Patch Set 1 #

Patch Set 2 : handling local and utc times + unittests #

Total comments: 25

Patch Set 3 : overloaded FromUTCExplode and FromLocalExplode for transition process if accepted #

Patch Set 4 : #

Total comments: 22

Patch Set 5 : mmenke comments #

Total comments: 16

Patch Set 6 : #

Total comments: 5

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -17 lines) Patch
M base/time/time.h View 1 2 3 4 5 6 4 chunks +30 lines, -4 lines 0 comments Download
M base/time/time.cc View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M base/time/time_mac.cc View 1 2 3 4 5 3 chunks +24 lines, -4 lines 0 comments Download
M base/time/time_posix.cc View 1 2 3 4 5 2 chunks +21 lines, -3 lines 0 comments Download
M base/time/time_unittest.cc View 1 2 3 4 5 1 chunk +46 lines, -0 lines 0 comments Download
M base/time/time_win.cc View 1 2 3 4 3 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 63 (25 generated)
maksims (do not use this acc)
Would you please take a look?
3 years, 5 months ago (2016-05-18 06:11:00 UTC) #6
mmenke
This looks very reasonable to me. https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcode1 base/time/time.h:1: // Copyright (c) ...
3 years, 5 months ago (2016-05-18 17:32:01 UTC) #8
Mark Mentovai
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc File base/time/time.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.cc#newcode348 base/time/time.cc:348: return (month != rhs.month || day_of_month != rhs.day_of_month || ...
3 years, 5 months ago (2016-05-18 17:42:49 UTC) #9
eroman
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcode423 base/time/time.h:423: bool operator!=(const Exploded& rhs); I don't think we want ...
3 years, 5 months ago (2016-05-18 19:35:18 UTC) #10
Mark Mentovai
https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time.h#newcode423 base/time/time.h:423: bool operator!=(const Exploded& rhs); On 2016/05/18 19:35:17, eroman wrote: ...
3 years, 5 months ago (2016-05-18 20:07:15 UTC) #11
mmenke
https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/1988663002/diff/60001/base/time/time_mac.cc#newcode193 base/time/time_mac.cc:193: // on a 28-30 day month, it will return ...
3 years, 5 months ago (2016-05-18 20:11:32 UTC) #12
Mark Mentovai
An alternative would be to have the unexploder match the highest common denominator on platforms ...
3 years, 5 months ago (2016-05-18 20:16:47 UTC) #13
mmenke
On 2016/05/18 20:16:47, Mark Mentovai wrote: > An alternative would be to have the unexploder ...
3 years, 5 months ago (2016-05-18 21:04:35 UTC) #14
Mark Mentovai
mmenke wrote: > On 2016/05/18 20:16:47, Mark Mentovai wrote: > > An alternative would be ...
3 years, 5 months ago (2016-05-18 21:07:57 UTC) #15
maksims (do not use this acc)
Would you please take a look? As eroman proposed, I made FromUTCExplode and FromLocalExplode to ...
3 years, 5 months ago (2016-05-19 09:57:20 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988663002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988663002/100001
3 years, 5 months ago (2016-05-19 17:48:37 UTC) #19
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/190682)
3 years, 5 months ago (2016-05-19 18:50:58 UTC) #21
mmenke
Hrm, looks like everywhere but Windows has the same behavior here.
3 years, 5 months ago (2016-05-19 19:14:31 UTC) #22
maksims (do not use this acc)
Windows x86 and x64 hit NOTREACHED() on failure and [5516:7412:0520/014754:23213650:FATAL:time_win.cc(295)] Check failed: false. Unable to ...
3 years, 5 months ago (2016-05-20 10:18:48 UTC) #25
maksims (do not use this acc)
On 2016/05/20 10:18:48, maksims wrote: > Windows x86 and x64 hit NOTREACHED() on failure and ...
3 years, 5 months ago (2016-05-20 10:19:26 UTC) #26
mmenke
https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newcode533 base/time/time.h:533: static bool FromUTCExploded(const Exploded& exploded, Time& time) { Per ...
3 years, 5 months ago (2016-05-20 19:44:57 UTC) #27
maksims (do not use this acc)
please take a look https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/160001/base/time/time.h#newcode533 base/time/time.h:533: static bool FromUTCExploded(const Exploded& exploded, ...
3 years, 4 months ago (2016-05-24 09:14:22 UTC) #30
mmenke
I'm happy with this, just nits. https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newcode523 base/time/time.h:523: // into a ...
3 years, 4 months ago (2016-05-24 16:08:33 UTC) #31
Mark Mentovai
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newcode428 base/time/time.h:428: bool operator!=(const Exploded& rhs); Put a blank line before ...
3 years, 4 months ago (2016-05-24 18:27:27 UTC) #32
mmenke
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newcode428 base/time/time.h:428: bool operator!=(const Exploded& rhs); On 2016/05/24 18:27:27, Mark Mentovai ...
3 years, 4 months ago (2016-05-24 18:31:19 UTC) #33
Mark Mentovai
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newcode428 base/time/time.h:428: bool operator!=(const Exploded& rhs); mmenke wrote: > On 2016/05/24 ...
3 years, 4 months ago (2016-05-24 18:34:13 UTC) #34
maksims (do not use this acc)
https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/220001/base/time/time.h#newcode428 base/time/time.h:428: bool operator!=(const Exploded& rhs); On 2016/05/24 18:34:13, Mark Mentovai ...
3 years, 4 months ago (2016-05-26 04:38:18 UTC) #40
Mark Mentovai
https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newcode527 base/time/time.h:527: ignore_result(FromExploded(false, exploded, &time)); Centralize the “false”/“true”. This should be ...
3 years, 4 months ago (2016-05-26 14:08:39 UTC) #41
mmenke
https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newcode606 base/time/time.h:606: static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs); On ...
3 years, 4 months ago (2016-05-26 14:45:08 UTC) #42
maksims (do not use this acc)
ptal https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/1988663002/diff/320001/base/time/time.h#newcode606 base/time/time.h:606: static bool ExplodedMostlyEquals(const Exploded& lhs, const Exploded& rhs); ...
3 years, 4 months ago (2016-05-27 10:19:09 UTC) #43
Mark Mentovai
Since it’s private, it’s not the end of the world to leave it where it ...
3 years, 4 months ago (2016-05-27 15:13:04 UTC) #44
mmenke
LGTM
3 years, 4 months ago (2016-05-27 15:22:56 UTC) #45
Mark Mentovai
LGTM
3 years, 4 months ago (2016-05-27 19:44:14 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988663002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988663002/340001
3 years, 4 months ago (2016-05-28 07:20:40 UTC) #48
commit-bot: I haz the power
Committed patchset #7 (id:340001)
3 years, 4 months ago (2016-05-28 09:20:29 UTC) #50
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382 Cr-Commit-Position: refs/heads/master@{#396638}
3 years, 4 months ago (2016-05-28 09:22:00 UTC) #52
Guido Urdaneta
A revert of this CL (patchset #7 id:340001) has been created in https://codereview.chromium.org/2022913002/ by guidou@chromium.org. ...
3 years, 4 months ago (2016-05-31 08:18:45 UTC) #53
Guido Urdaneta
On 2016/05/31 08:18:45, Guido Urdaneta wrote: > A revert of this CL (patchset #7 id:340001) ...
3 years, 4 months ago (2016-05-31 12:34:10 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988663002/340001
3 years, 4 months ago (2016-06-14 19:50:19 UTC) #57
commit-bot: I haz the power
Committed patchset #7 (id:340001)
3 years, 4 months ago (2016-06-14 21:57:22 UTC) #59
commit-bot: I haz the power
CQ bit was unchecked
3 years, 4 months ago (2016-06-14 21:57:45 UTC) #60
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/558f168054566ecbc611918093398add04dc13a4 Cr-Commit-Position: refs/heads/master@{#399794}
3 years, 4 months ago (2016-06-14 22:00:21 UTC) #62
pdknsk
3 years, 1 month ago (2016-08-25 19:33:12 UTC) #63
Message was sent while issue was closed.
This partially broke FTP directory listings on Linux (and perhaps Mac).

http://crbug.com/641096

Powered by Google App Engine
This is Rietveld 408576698