Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1072913003:
Fix "security certificate expired 0 day(s) ago" bug (Closed)
DescriptionFix "security certificate expired 0 day(s) ago" bug
Make sure the expiration value of an SSL certificate is correctly
displayed in an interstitial.
BUG=476758
Committed: https://crrev.com/0757a44c1e8bd955c145941aef8ba30ee43bc781
Cr-Commit-Position: refs/heads/master@{#326718}
Patch Set 1 : Correctly display the expiration value of a certificate with an invalid date. #
Total comments: 1
Patch Set 2 : Fix include order. #Patch Set 3 : Fix merge conflict #Patch Set 4 : Remove std::ceil #
Total comments: 6
Patch Set 5 : Felt comments #Messages
Total messages: 35 (14 generated)
Patchset #1 (id:1) has been deleted
fahl@chromium.org changed reviewers: + felt@chromium.org, lgarron@chromium.org
On 2015/04/21 00:14:58, fahl wrote: You should give this CL a real subject line :)
On 2015/04/21 02:21:18, felt wrote: > On 2015/04/21 00:14:58, fahl wrote: > > You should give this CL a real subject line :) Done :)
On 2015/04/21 at 00:14:58, fahl wrote: > LGTM. (I was conflicted about whether something expiring 24.5 hours ago on the previous calendar date should be "two days", but you can't really win here. Opposite case: it's 1am, and the cert expired 26 hours ago. "One day" or "two days"? Considering that rounding to days based on the number of hours is always going to have this issue one way or another, we might as well make sense if it's under 24 hours.)
On 2015/04/21 02:22:55, fahl wrote: > On 2015/04/21 02:21:18, felt wrote: > > On 2015/04/21 00:14:58, fahl wrote: > > > > You should give this CL a real subject line :) > > Done :) It still says "chrome/browser/ssl/ssl_error_info.cc" at the top...
On 2015/04/21 at 02:31:42, felt wrote: > On 2015/04/21 02:22:55, fahl wrote: > > On 2015/04/21 02:21:18, felt wrote: > > > On 2015/04/21 00:14:58, fahl wrote: > > > > > > You should give this CL a real subject line :) > > > > Done :) > > It still says "chrome/browser/ssl/ssl_error_info.cc" at the top... I see `Fix "security certificate expired 0 day(s) ago" bug`.
On 2015/04/21 02:38:07, lgarron wrote: > On 2015/04/21 at 02:31:42, felt wrote: > > On 2015/04/21 02:22:55, fahl wrote: > > > On 2015/04/21 02:21:18, felt wrote: > > > > On 2015/04/21 00:14:58, fahl wrote: > > > > > > > > You should give this CL a real subject line :) > > > > > > Done :) > > > > It still says "chrome/browser/ssl/ssl_error_info.cc" at the top... > > I see `Fix "security certificate expired 0 day(s) ago" bug`. Maybe there's a consistency issue. I still see the old string. :( The subject line needs to be repeated as the first line of the description, can you pls add it there too?
On 2015/04/21 02:43:45, felt wrote: > On 2015/04/21 02:38:07, lgarron wrote: > > On 2015/04/21 at 02:31:42, felt wrote: > > > On 2015/04/21 02:22:55, fahl wrote: > > > > On 2015/04/21 02:21:18, felt wrote: > > > > > On 2015/04/21 00:14:58, fahl wrote: > > > > > > > > > > You should give this CL a real subject line :) > > > > > > > > Done :) > > > > > > It still says "chrome/browser/ssl/ssl_error_info.cc" at the top... > > > > I see `Fix "security certificate expired 0 day(s) ago" bug`. > > Maybe there's a consistency issue. I still see the old string. :( > > The subject line needs to be repeated as the first line of the description, can > you pls add it there too? Ok, done. Looks like you still have the first version cached?!
lgtm % nit https://codereview.chromium.org/1072913003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_info.cc (right): https://codereview.chromium.org/1072913003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_info.cc:4: #include <cmath> please fix include order: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, felt@chromium.org Link to the patchset: https://codereview.chromium.org/1072913003/#ps40001 (title: "Fix include order.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072913003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, felt@chromium.org Link to the patchset: https://codereview.chromium.org/1072913003/#ps60001 (title: "Fix merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072913003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
Patchset #4 (id:80001) has been deleted
On 2015/04/23 01:52:35, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) felt and lgarron: Could you please have another look? I removed std::ceil since it's a library and not a language feature and did not properly compile on some platforms. I actually figured out that the computation of the correct expiration value always returns a too little value, i.e. 0 although it should be 1 but also 10 although it should be 11 days. Simply adding 1 to the current expiration value should fix that problem.
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, felt@chromium.org Link to the patchset: https://codereview.chromium.org/1072913003/#ps100001 (title: "Remove std::ceil")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072913003/100001
https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_info.cc (right): https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:8: #include "chrome/browser/ssl/ssl_error_info.h" please undo this header reordering -- if you check out the include order in the style guide, the associated .h file is supposed to be on its own line above (like it was before) https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:62: // the expiration value (https://crbug.com/476758). this comment says it's still ceil-ing, but it isn't https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:64: (base::Time::Now() - cert->valid_expiry()).InDays() + 1; this doesn't look like it's indented correctly
The CQ bit was unchecked by felt@chromium.org
https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_info.cc (right): https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:8: #include "chrome/browser/ssl/ssl_error_info.h" On 2015/04/24 00:19:24, felt wrote: > please undo this header reordering -- if you check out the include order in the > style guide, the associated .h file is supposed to be on its own line above > (like it was before) Done. https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:62: // the expiration value (https://crbug.com/476758). On 2015/04/24 00:19:24, felt wrote: > this comment says it's still ceil-ing, but it isn't Done. https://codereview.chromium.org/1072913003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_info.cc:64: (base::Time::Now() - cert->valid_expiry()).InDays() + 1; On 2015/04/24 00:19:24, felt wrote: > this doesn't look like it's indented correctly Done.
lgtm f'real now
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org Link to the patchset: https://codereview.chromium.org/1072913003/#ps120001 (title: "Felt comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072913003/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0757a44c1e8bd955c145941aef8ba30ee43bc781 Cr-Commit-Position: refs/heads/master@{#326718} |
