Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(485)

Issue 2090713003: Make callers of FromUTC(Local)Exploded in net/ use new time API. (Closed)

Created:
4 years, 5 months ago by maksims (do not use this acc)
Modified:
3 years, 12 months ago
Reviewers:
maksims_, eroman, mmenke
CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri, Paweł Hajdan Jr., kinuko+cache_chromium.org, gavinp+disk_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905, 601903, 601900 Committed: https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648 Cr-Commit-Position: refs/heads/master@{#439789}

Patch Set 1 #

Patch Set 2 : //net #

Patch Set 3 : rebased #

Total comments: 1

Patch Set 4 : Make callers of FromUTC(Local)Exploded in net/ use new time API. #

Total comments: 4

Patch Set 5 : revert #

Total comments: 9

Patch Set 6 : Comments from eroman@ #

Total comments: 10

Patch Set 7 : commetns from eroman #

Patch Set 8 : fix typo #

Patch Set 9 : ... #

Patch Set 10 : revert ftp_util #

Total comments: 4

Patch Set 11 : remove redundant line #

Total comments: 4

Patch Set 12 : fix X509 parse date test on 32-bit machines #

Patch Set 13 : fix accidently changed line while rebasing - use UTCExploded instead of LocalExploded #

Patch Set 14 : fix accidently changed line while rebasing - use UTCExploded instead of LocalExploded #

Patch Set 15 : use EXPECT_TRUE #

Total comments: 6

Patch Set 16 : comments #

Patch Set 17 : typo #

Total comments: 2

Patch Set 18 : undo comment change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -68 lines) Patch
M net/cert/ct_policy_enforcer_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +47 lines, -28 lines 0 comments Download
M net/cert/x509_cert_types.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -5 lines 0 comments Download
M net/cert/x509_cert_types_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +23 lines, -14 lines 0 comments Download
M net/disk_cache/blockfile/eviction.cc View 1 2 3 4 5 1 chunk +4 lines, -6 lines 0 comments Download
M net/extras/sqlite/sqlite_channel_id_store_unittest.cc View 1 2 3 2 chunks +6 lines, -2 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_ls.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -2 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_unittest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -1 line 0 comments Download
M net/ftp/ftp_directory_listing_parser_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M net/ftp/ftp_directory_listing_parser_vms.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -2 lines 0 comments Download
M net/ftp/ftp_util.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -4 lines 0 comments Download
M net/http/http_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -2 lines 0 comments Download

Messages

Total messages: 132 (107 generated)
maksims (do not use this acc)
eroman@, mmenke@, basically, this cl had issues as well. I've just forgotten about this.
4 years, 2 months ago (2016-10-05 07:15:08 UTC) #26
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/160001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/160001/net/ftp/ftp_util.cc#newcode288 net/ftp/ftp_util.cc:288: // We don't know the time zone of the ...
4 years, 2 months ago (2016-10-05 12:59:02 UTC) #29
mmenke
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#newcode288 net/ftp/ftp_util.cc:288: // We don't know the time zone of the ...
4 years, 2 months ago (2016-10-05 16:53:39 UTC) #32
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#newcode288 net/ftp/ftp_util.cc:288: // We don't know the time zone of the ...
4 years, 2 months ago (2016-10-06 12:07:07 UTC) #33
mmenke
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#newcode288 net/ftp/ftp_util.cc:288: // We don't know the time zone of the ...
4 years, 2 months ago (2016-10-06 15:36:54 UTC) #34
eroman
In general I am supportive of the changes https://codereview.chromium.org/2090713003/diff/220001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/cert_verify_proc.cc#newcode697 net/cert/cert_verify_proc.cc:697: if ...
4 years, 2 months ago (2016-10-20 18:05:21 UTC) #53
maksims (do not use this acc)
mmenke@, eroman@, there is a problem with Time::FromExploded. Especially with the following line - https://cs.chromium.org/chromium/src/base/time/time_posix.cc?sq=package:chromium&dr=C&rcl=1477451763&l=256 ...
4 years, 1 month ago (2016-10-26 08:16:40 UTC) #63
eroman
looks good after these comments. I didn't quite understand your question regarding SysTimeFromTimeStruct -- could ...
4 years, 1 month ago (2016-10-27 21:44:35 UTC) #64
maksims (do not use this acc)
On 2016/10/27 21:44:35, eroman (slow) wrote: > looks good after these comments. > > I ...
4 years, 1 month ago (2016-11-15 10:29:12 UTC) #81
eroman
I believe the problem you are running into has to do with the sandbox on ...
4 years, 1 month ago (2016-11-17 23:21:09 UTC) #82
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc#newcode74 net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/10/27 21:44:34, eroman (vacation - ...
4 years ago (2016-11-29 07:38:57 UTC) #85
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#newcode290 net/ftp/ftp_util.cc:290: return true; eroman@, can we leave these parts as ...
4 years ago (2016-11-29 08:41:18 UTC) #88
eroman
https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc#newcode74 net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/11/29 07:38:56, maksims wrote: > ...
4 years ago (2016-11-30 00:21:05 UTC) #89
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_types.cc#newcode74 net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/11/30 00:21:05, eroman (slow) wrote: ...
4 years ago (2016-12-05 13:15:49 UTC) #91
eroman
https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#newcode290 net/ftp/ftp_util.cc:290: return true; On 2016/12/05 13:15:49, maksims wrote: > On ...
4 years ago (2016-12-08 01:39:19 UTC) #92
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_types.cc File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_types.cc#newcode77 net/cert/x509_cert_types.cc:77: // FromUTCExploded() can fail even though exploded.HasValidValues() On 2016/12/08 ...
4 years ago (2016-12-13 08:56:10 UTC) #94
eroman
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc#newcode213 net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) Won't "out_time.ToInternalValue() <= ...
4 years ago (2016-12-17 02:39:30 UTC) #110
maksims_
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc#newcode213 net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/17 02:39:30, ...
4 years ago (2016-12-19 15:45:13 UTC) #112
eroman
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc#newcode213 net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/19 15:45:13, ...
4 years ago (2016-12-19 19:22:31 UTC) #113
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_types_unittest.cc#newcode213 net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/19 19:22:31, ...
4 years ago (2016-12-19 20:37:32 UTC) #114
eroman
lgtm https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_listing_parser_vms.cc File net/ftp/ftp_directory_listing_parser_vms.cc (right): https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_listing_parser_vms.cc#newcode195 net/ftp/ftp_directory_listing_parser_vms.cc:195: // We don't know the time zone of ...
3 years, 12 months ago (2016-12-19 22:42:35 UTC) #123
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2090713003/540001
3 years, 12 months ago (2016-12-20 12:32:37 UTC) #126
maksims (do not use this acc)
https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_listing_parser_vms.cc File net/ftp/ftp_directory_listing_parser_vms.cc (right): https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_listing_parser_vms.cc#newcode195 net/ftp/ftp_directory_listing_parser_vms.cc:195: // We don't know the time zone of the ...
3 years, 12 months ago (2016-12-20 12:32:49 UTC) #127
commit-bot: I haz the power
Committed patchset #18 (id:540001)
3 years, 12 months ago (2016-12-20 13:36:34 UTC) #130
commit-bot: I haz the power
3 years, 12 months ago (2016-12-20 13:38:58 UTC) #132
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648
Cr-Commit-Position: refs/heads/master@{#439789}

Powered by Google App Engine
This is Rietveld 408576698