Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657433002:
Use upstream version of PR_ImplodeTime (Closed)
|
Created:
4 years, 10 months ago by pkl (ping after 24h if needed) Modified:
4 years, 10 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. |
DescriptionUse upstream version of PR_ImplodeTime
BUG=567982
Committed: https://crrev.com/e19b0f23b9c899b4430e092ca4bef826aedc8b55
Cr-Commit-Position: refs/heads/master@{#375481}
Patch Set 1 #Patch Set 2 : comments and formatting #Patch Set 3 : formatting #Patch Set 4 : git cl format #Patch Set 5 : rebase #Patch Set 6 : comment attributing source #
Total comments: 10
Patch Set 7 : addressed comments from wtc #Messages
Total messages: 26 (11 generated)
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/1657433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657433002/40001
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.
pkl@chromium.org changed reviewers: + sdefresne@chromium.org, thakis@chromium.org
This is a spinoff from https://codereview.chromium.org/1494083005/ , addressing just the prtime.cc implementation.
Description was changed from ========== Use upstream version of PR_ImplodeTime BUG=565574 ========== to ========== Use upstream version of PR_ImplodeTime BUG=567982 ==========
lgtm Nico, could you take a look too? Landing this would greatly help us in our effort of unforking Chrome on iOS. Thank you!
rs-lgtm I asked wtc about this a while ago, here's our exchange: """ thakis: > Hi, > > I have a question about an 8 year old change: > https://chrome-internal.googlesource.com/chrome/chrome-history/+/d3a8fb586fb0... > > Do you remember why you changed PR_ImplodeTime to use windows api functions > instead of keeping nspr's original code for this? Ananta doesn't remember, > but he said you might :-) wtc: Hi Nico, 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. """
cc wtc fyi (nothing to do here for you; see the previous comment if you want to look at this at all)
wtc@chromium.org changed reviewers: + wtc@chromium.org
Patch set 6 LGTM. I know the upstream code very well and am happy to review the CL. I suggest some small changes. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:57: * and the timezone offsets are applied to the FILETIME structure. Please delete these two lines. They describe the old code. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:78: * Long-long (64-bit signed integer type) support functions Nit: functions => macros https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:85: #define LL_SUB(r, a, b) ((r) = (a) - (b)) Optional: Based on the comment at lines 58-63, these could be moved to base/third_party/prtime.h. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:122: * http://lxr.mozilla.org/nspr/source/pr/src/misc/prtime.c#221 This note can be omitted or should be moved to the beginning of the file, around line 44 or line 56. (I would just omit it.) https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:188: Nit/optional: To match the upstream source file closer, the PR_ImplodeTime function should be positioned here, right before the IsLeapYear function.
On 2016/02/12 15:18:19, Nico wrote: > > wtc: > Hi Nico, > [...] > > 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. We should look in base/time/pr_time_unittest.cc to see if there is any unit test that specifically tests how we handle this condition on 32-bit Linux. If so, that unit test needs to be updated or removed.
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/1657433002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657433002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/15 at 10:57:02, commit-bot wrote: > Dry run: This issue passed the CQ dry run. wtc: looks like all the tests pass with the CL, so I guess there was no tests depending on the previous implementation regarding year 2038.
patch 7 addressed wtc's comments. I will land that via CQ now. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... File base/third_party/nspr/prtime.cc (right): https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:57: * and the timezone offsets are applied to the FILETIME structure. On 2016/02/12 16:15:32, wtc wrote: > > Please delete these two lines. They describe the old code. Done. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:78: * Long-long (64-bit signed integer type) support functions On 2016/02/12 16:15:32, wtc wrote: > > Nit: functions => macros Done. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:85: #define LL_SUB(r, a, b) ((r) = (a) - (b)) On 2016/02/12 16:15:32, wtc wrote: > > Optional: Based on the comment at lines 58-63, these could be moved to > base/third_party/prtime.h. I would rather defer this move until a later time. https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:122: * http://lxr.mozilla.org/nspr/source/pr/src/misc/prtime.c#221 On 2016/02/12 16:15:32, wtc wrote: > > This note can be omitted or should be moved to the beginning of the file, around > line 44 or line 56. (I would just omit it.) I moved it to the beginning of the file (around line 56). https://codereview.chromium.org/1657433002/diff/100001/base/third_party/nspr/... base/third_party/nspr/prtime.cc:188: On 2016/02/12 16:15:32, wtc wrote: > > Nit/optional: To match the upstream source file closer, the PR_ImplodeTime > function should be positioned here, right before the IsLeapYear function. Done.
The CQ bit was checked by pkl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, wtc@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1657433002/#ps120001 (title: "addressed comments from wtc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657433002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657433002/120001
Message was sent while issue was closed.
Description was changed from ========== Use upstream version of PR_ImplodeTime BUG=567982 ========== to ========== Use upstream version of PR_ImplodeTime BUG=567982 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Use upstream version of PR_ImplodeTime BUG=567982 ========== to ========== Use upstream version of PR_ImplodeTime BUG=567982 Committed: https://crrev.com/e19b0f23b9c899b4430e092ca4bef826aedc8b55 Cr-Commit-Position: refs/heads/master@{#375481} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e19b0f23b9c899b4430e092ca4bef826aedc8b55 Cr-Commit-Position: refs/heads/master@{#375481} |
