Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
376663002:
Calculate severity score for date_invalid error (Closed)
|
Created:
6 years, 5 months ago by radhikabhar Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCalculate severity score for date_invalid error
BUG=390346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282878
Patch Set 1 #Patch Set 2 : Added test cases #
Total comments: 8
Patch Set 3 : Changes #Patch Set 4 : Fixed typo #
Total comments: 23
Patch Set 5 : Addressed comments #
Total comments: 8
Patch Set 6 : Addressed comments #
Total comments: 26
Patch Set 7 : Addressed comments #
Total comments: 6
Patch Set 8 : Addressed comments #
Total comments: 10
Patch Set 9 : Addressed comments #Patch Set 10 : Fixed compilation errors #
Messages
Total messages: 23 (0 generated)
This just calculates the score. I haven't called the function yet.
https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... File chrome/browser/ssl/ssl_severity_date_invalid.cc (right): https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.cc:54: float SSLSeverityDateInvalid::IsUserClockWrong() { It doesn't make sense to have both ::IsUserClockWrong and ::IsErrorProbablyCausedByClock. Can you combine the two? https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.cc:61: network_time = base::GetBuildTime(); Why not check *both* the build time and the network time? Then return true if either one fails. https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... File chrome/browser/ssl/ssl_severity_date_invalid.h (right): https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.h:13: // severity score when the SSL Error is Date Invalid. ^ make sure to read your comments, there are two big typos https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.h:14: class SSLSeverityDateInvalid { I don't think you need to make a new class for each type of error. You're going to end up wanting to reuse things, like whether the user has visited the page before, across different error types. Instead, it probably makes sense to make a single class that has a whole bunch of generic helper functions. For example: class SSLErrorClassification { private: bool IsUserClockWrong(); } Then you can later add methods that make use of the helper functions, like: float ScoreDateInvalid() { float score = 0; if (IsUserClockWrong()) score = score + .4; } That way you can also have a method like: void RecordUMAStatistics() { if (IsUserClockWrong()) { // record UMA stat } if (IsItCloudyToday()) { // record UMA stat } } You can also move the SSLInterstitialCause and RecordSSLInterstitialCause here, from the ssl_blocking_page.cc. Then have SSLBlockingPage::GetHTMLContentsV2 invoke your method to see whether the user's time wrong.
https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... File chrome/browser/ssl/ssl_severity_date_invalid.cc (right): https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.cc:54: float SSLSeverityDateInvalid::IsUserClockWrong() { On 2014/07/07 22:01:34, felt wrote: > It doesn't make sense to have both ::IsUserClockWrong and > ::IsErrorProbablyCausedByClock. Can you combine the two? Done. https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.cc:61: network_time = base::GetBuildTime(); On 2014/07/07 22:01:34, felt wrote: > Why not check *both* the build time and the network time? Then return true if > either one fails. Done. https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... File chrome/browser/ssl/ssl_severity_date_invalid.h (right): https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.h:13: // severity score when the SSL Error is Date Invalid. On 2014/07/07 22:01:34, felt wrote: > ^ make sure to read your comments, there are two big typos Done. https://codereview.chromium.org/376663002/diff/20001/chrome/browser/ssl/ssl_s... chrome/browser/ssl/ssl_severity_date_invalid.h:14: class SSLSeverityDateInvalid { On 2014/07/07 22:01:34, felt wrote: > I don't think you need to make a new class for each type of error. You're going > to end up wanting to reuse things, like whether the user has visited the page > before, across different error types. > > Instead, it probably makes sense to make a single class that has a whole bunch > of generic helper functions. For example: > > class SSLErrorClassification { > private: > bool IsUserClockWrong(); > } > > Then you can later add methods that make use of the helper functions, like: > > float ScoreDateInvalid() { > float score = 0; > if (IsUserClockWrong()) score = score + .4; > } > > That way you can also have a method like: > > void RecordUMAStatistics() { > if (IsUserClockWrong()) { > // record UMA stat > } > if (IsItCloudyToday()) { > // record UMA stat > } > } > > You can also move the SSLInterstitialCause and RecordSSLInterstitialCause here, > from the ssl_blocking_page.cc. Then have SSLBlockingPage::GetHTMLContentsV2 > invoke your method to see whether the user's time wrong. Done.
A few more comments https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:26: }; ^ woah crazy formatting here :) https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:44: cert_(cert) { } formatting https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:48: float SSLErrorClassification::ServerCharacteristics(){ missing space: ServerCharacteristics() { https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: if (cert_->HasExpired()) { you don't need to use { } for single-line if-statements https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:65: //TODO(radhikabhar): Check website settings. need a space: // TODO https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:69: float SSLErrorClassification::TimePassedSinceExpiry() { It doesn't make sense for a method named TimePassedSinceExpiry() to return a float that is a score (and not an amount of time). A) Create a new method named SSLErrorClassification::CalculateScoreTimePassedSinceExpiry, which will be used only for scoring in a single way. B) Keep the method name ::TimePassedSinceExpiry() and have it return a base::TimeDelta, so it can be reused in multiple places. And then the caller is responsible for doing the scoring. This would allow the time calculation to be reused in several places. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:90: // machine's build time. you ought to check both network time and build time. network time could be wrong, even if it's initialized. if (time_now < network_time || time_now < build_time) return true; then you wouldn't need the (network_time < build_time) check, I don't think? nor would you need a network_time > build_time check (which I don't see here) https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:113: } same comment as above for checking both build_time and network_time https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:13: // severity score when the SSL Error is Date Invalid. this class doesn't have to be specific to Date Invalid. can you update the comment? https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:15: public : the spacing should be public: // one space float ... // two spaces https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:29: ~SSLErrorClassification(); I don't think these methods are in the right order. Can you look at the C++ style guide google-styleguide.googlecode.com/svn/trunk/cppguide.xml and make sure they are ordered correctly? https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:32: nit: no enter here
On 2014/07/09 19:00:45, felt wrote: > A few more comments > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.cc (right): > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:26: }; > ^ woah crazy formatting here :) > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:44: cert_(cert) { } > formatting > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:48: float > SSLErrorClassification::ServerCharacteristics(){ > missing space: > ServerCharacteristics() { > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:49: if (cert_->HasExpired()) { > you don't need to use { } for single-line if-statements > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:65: //TODO(radhikabhar): Check > website settings. > need a space: > // TODO > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:69: float > SSLErrorClassification::TimePassedSinceExpiry() { > It doesn't make sense for a method named TimePassedSinceExpiry() to return a > float that is a score (and not an amount of time). > > A) Create a new method named > SSLErrorClassification::CalculateScoreTimePassedSinceExpiry, which will be used > only for scoring in a single way. > B) Keep the method name ::TimePassedSinceExpiry() and have it return a > base::TimeDelta, so it can be reused in multiple places. And then the caller is > responsible for doing the scoring. This would allow the time calculation to be > reused in several places. > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:90: // machine's build time. > you ought to check both network time and build time. network time could be > wrong, even if it's initialized. > > if (time_now < network_time || time_now < build_time) return true; > > then you wouldn't need the (network_time < build_time) check, I don't think? nor > would you need a network_time > build_time check (which I don't see here) > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.cc:113: } > same comment as above for checking both build_time and network_time > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_classification.h (right): > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:13: // severity score when the SSL > Error is Date Invalid. > this class doesn't have to be specific to Date Invalid. can you update the > comment? > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:15: public : > the spacing should be > > public: // one space > float ... // two spaces > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:29: ~SSLErrorClassification(); > I don't think these methods are in the right order. Can you look at the C++ > style guide google-styleguide.googlecode.com/svn/trunk/cppguide.xml and make > sure they are ordered correctly? > > https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_classification.h:32: > nit: no enter here PS you should use git cl format to help with style issues
https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:26: }; On 2014/07/09 19:00:45, felt wrote: > ^ woah crazy formatting here :) Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:44: cert_(cert) { } On 2014/07/09 19:00:44, felt wrote: > formatting Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:48: float SSLErrorClassification::ServerCharacteristics(){ On 2014/07/09 19:00:45, felt wrote: > missing space: > ServerCharacteristics() { Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: if (cert_->HasExpired()) { On 2014/07/09 19:00:44, felt wrote: > you don't need to use { } for single-line if-statements Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:65: //TODO(radhikabhar): Check website settings. On 2014/07/09 19:00:45, felt wrote: > need a space: > // TODO Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:69: float SSLErrorClassification::TimePassedSinceExpiry() { On 2014/07/09 19:00:45, felt wrote: > It doesn't make sense for a method named TimePassedSinceExpiry() to return a > float that is a score (and not an amount of time). > > A) Create a new method named > SSLErrorClassification::CalculateScoreTimePassedSinceExpiry, which will be used > only for scoring in a single way. > B) Keep the method name ::TimePassedSinceExpiry() and have it return a > base::TimeDelta, so it can be reused in multiple places. And then the caller is > responsible for doing the scoring. This would allow the time calculation to be > reused in several places. Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:90: // machine's build time. I have deleted this because according to rsleevi@ comment, you cannot use the network_time_tracker for security purposes. Details are here - https://code.google.com/p/chromium/issues/detail?id=392662 On 2014/07/09 19:00:44, felt wrote: > you ought to check both network time and build time. network time could be > wrong, even if it's initialized. > > if (time_now < network_time || time_now < build_time) return true; > > then you wouldn't need the (network_time < build_time) check, I don't think? nor > would you need a network_time > build_time check (which I don't see here) https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:113: } Same as above. On 2014/07/09 19:00:44, felt wrote: > same comment as above for checking both build_time and network_time https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:13: // severity score when the SSL Error is Date Invalid. On 2014/07/09 19:00:45, felt wrote: > this class doesn't have to be specific to Date Invalid. can you update the > comment? Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:29: ~SSLErrorClassification(); On 2014/07/09 19:00:45, felt wrote: > I don't think these methods are in the right order. Can you look at the C++ > style guide google-styleguide.googlecode.com/svn/trunk/cppguide.xml and make > sure they are ordered correctly? Done. https://codereview.chromium.org/376663002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:32: On 2014/07/09 19:00:45, felt wrote: > nit: no enter here Done.
https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:23: CLOCK_PAST, nit: how many spaces in the indentation here? it looks wrong https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:94: RecordSSLInterstitialCause(overridable, CLOCK_PAST); since IsUserClockInThePast() has multiple call sites, it doesn't make sense to do the UMA stat inside the method. can you add another method like: void SSLErrorClassification::RecordUMAStatistics(bool overridable) { IsUserClockInThePast(...); IsUserClockInTheFuture(...); } then invoke SSLErrorClassificaiton::RecordUMAStatistics() from the SSLBlockingPage() constructor. as a bonus, this means that IsUserClockInThePast and IsUserClockInTheFuture no longer need to take the overridable bool in the method name https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:16: nit: a single line after public public: SSLErrorClassification(... https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:40: FRIEND_TEST_ALL_PREFIXES(SSLErrorClassification, TestDateInvalidScore); Usually put friend classes at the very beginning of the private: declaration.
https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:23: CLOCK_PAST, On 2014/07/10 18:41:10, felt wrote: > nit: how many spaces in the indentation here? it looks wrong Done. https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:94: RecordSSLInterstitialCause(overridable, CLOCK_PAST); On 2014/07/10 18:41:10, felt wrote: > since IsUserClockInThePast() has multiple call sites, it doesn't make sense to > do the UMA stat inside the method. can you add another method like: > > void SSLErrorClassification::RecordUMAStatistics(bool overridable) { > IsUserClockInThePast(...); > IsUserClockInTheFuture(...); > } > > then invoke SSLErrorClassificaiton::RecordUMAStatistics() from the > SSLBlockingPage() constructor. as a bonus, this means that IsUserClockInThePast > and IsUserClockInTheFuture no longer need to take the overridable bool in the > method name Done. https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:16: On 2014/07/10 18:41:10, felt wrote: > nit: a single line after public > > public: > SSLErrorClassification(... Done. https://codereview.chromium.org/376663002/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:40: FRIEND_TEST_ALL_PREFIXES(SSLErrorClassification, TestDateInvalidScore); On 2014/07/10 18:41:10, felt wrote: > Usually put friend classes at the very beginning of the private: declaration. Done.
https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:497: && (SSLErrorInfo::NetErrorToErrorType(cert_error_) == Style nit: Usually, more parentheses is more good, but in this case I think it'd be better to have just: && SSLErrorInfo::NetErrorToErrorType(cert_error_) == SSLErrorInfo::CERT_DATE_INVALID But, your call. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2014 https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: // Client side characterisitics. Check whether the user clock is wrong and nit: "...whether or not the system's clock is wrong and whether or not it has encountered this error before." https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: // Client side characterisitics. Check whether the user clock is wrong and Typo: "Client-side characteristics." https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:54: severity_date_score = 0.5 * 0.75 * 0.1; Perhaps explain these magic numbers by declaring them as static const variables with meaningful names. static const float kWrongClockBadness = 0.1; static const float kCorrectClockBadness = 1.0; // et c. ... https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:60: // Server Side characterisitics. Check whether the certificate has expired or Typo: "Server-side characteristics." https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:80: const int kHighThreshold = 7; These are going to get promoted to int64 when you do the comparison; it "feels" cleaner to me to declare them as the same type that you are going to compare them to. Not a big deal though. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:83: return 0.4; More magic numbers to name. :) https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2014, since this is a new file. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:17: net::X509Certificate* cert); Can this be: const net::x509Certificate& cert ? https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:24: // A function which calculates the severity score when the ssl error is Nit: I'd say "certificate error is |CERT_DATE_INVALID|" (or whatever the actual constant error value is). https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:26: float InvalidDateSeverityScore(); This member function can probably be declared const? float InvalidDateSeverityScore() const; https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:40: net::X509Certificate* cert_ ; Same thing: const & instead of non-const * (if possible).
https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:497: && (SSLErrorInfo::NetErrorToErrorType(cert_error_) == On 2014/07/10 21:40:04, Chromium Palmer wrote: > Style nit: Usually, more parentheses is more good, but in this case I think it'd > be better to have just: > > && SSLErrorInfo::NetErrorToErrorType(cert_error_) == > SSLErrorInfo::CERT_DATE_INVALID > > But, your call. Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/10 21:40:04, Chromium Palmer wrote: > 2014 Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: // Client side characterisitics. Check whether the user clock is wrong and On 2014/07/10 21:40:05, Chromium Palmer wrote: > nit: "...whether or not the system's clock is wrong and whether or not it has > encountered this error before." Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:49: // Client side characterisitics. Check whether the user clock is wrong and On 2014/07/10 21:40:05, Chromium Palmer wrote: > nit: "...whether or not the system's clock is wrong and whether or not it has > encountered this error before." Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:54: severity_date_score = 0.5 * 0.75 * 0.1; On 2014/07/10 21:40:04, Chromium Palmer wrote: > Perhaps explain these magic numbers by declaring them as static const variables > with meaningful names. > > static const float kWrongClockBadness = 0.1; > static const float kCorrectClockBadness = 1.0; > // et c. ... Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:60: // Server Side characterisitics. Check whether the certificate has expired or On 2014/07/10 21:40:05, Chromium Palmer wrote: > Typo: "Server-side characteristics." Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:80: const int kHighThreshold = 7; On 2014/07/10 21:40:04, Chromium Palmer wrote: > These are going to get promoted to int64 when you do the comparison; it "feels" > cleaner to me to declare them as the same type that you are going to compare > them to. Not a big deal though. Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:83: return 0.4; On 2014/07/10 21:40:05, Chromium Palmer wrote: > More magic numbers to name. :) Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/10 21:40:05, Chromium Palmer wrote: > 2014, since this is a new file. Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:17: net::X509Certificate* cert); On 2014/07/10 21:40:05, Chromium Palmer wrote: > Can this be: > > const net::x509Certificate& cert > > ? Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:24: // A function which calculates the severity score when the ssl error is On 2014/07/10 21:40:05, Chromium Palmer wrote: > Nit: I'd say "certificate error is |CERT_DATE_INVALID|" (or whatever the actual > constant error value is). Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:26: float InvalidDateSeverityScore(); On 2014/07/10 21:40:05, Chromium Palmer wrote: > This member function can probably be declared const? > > float InvalidDateSeverityScore() const; Done. https://codereview.chromium.org/376663002/diff/250007/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:40: net::X509Certificate* cert_ ; On 2014/07/10 21:40:05, Chromium Palmer wrote: > Same thing: const & instead of non-const * (if possible). Done.
https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:107: if (time_now < build_time) this code used to add a little buffer (2 days), in case of time zone changes or something else that made the time just a little wonky https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:112: bool SSLErrorClassification::IsUserClockInTheFuture(base::Time time_now) { In the .h file, you should probably clarify that this tells you **either** if the clock is in the future or if someone is using a super old version of chrome (a year old). https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:27: void RecordUMAStatistics(bool overridable); could this also be a static method, at least for now?
https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:107: if (time_now < build_time) On 2014/07/11 14:12:48, felt wrote: > this code used to add a little buffer (2 days), in case of time zone changes or > something else that made the time just a little wonky Done. https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:112: bool SSLErrorClassification::IsUserClockInTheFuture(base::Time time_now) { On 2014/07/11 14:12:48, felt wrote: > In the .h file, you should probably clarify that this tells you **either** if > the clock is in the future or if someone is using a super old version of chrome > (a year old). Done. https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/350001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:27: void RecordUMAStatistics(bool overridable); On 2014/07/11 14:12:48, felt wrote: > could this also be a static method, at least for now? Done. But in the future if I have to record any other cause as a UMA histogram for e.g. host name too broad for a wildcard certificate then I have will have to make it non-static.
LGTM, with some nits. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:48: float SSLErrorClassification::InvalidDateSeverityScore() const{ Niot: Space: "const {". https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:53: static const float kClientWeightage = 0.5; Nit: I'd go with "Weight" or "Weighting". https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:70: // TODO(radhikabhar): Check website settings. Does this refer to the "Server-side characteristics" code below? If there is more work to be done, file a bug for it and include a crbug.com/12345 link in the TODO comment. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:84: base::TimeDelta SSLErrorClassification::TimePassedSinceExpiry() const{ "const {" here and on line 89. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:16: const::net::X509Certificate& cert); Oops: No "::" between "const" and "net::". :)
https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:48: float SSLErrorClassification::InvalidDateSeverityScore() const{ On 2014/07/11 17:24:17, Chromium Palmer wrote: > Niot: Space: "const {". Done. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:53: static const float kClientWeightage = 0.5; On 2014/07/11 17:24:18, Chromium Palmer wrote: > Nit: I'd go with "Weight" or "Weighting". Done. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:70: // TODO(radhikabhar): Check website settings. On 2014/07/11 17:24:18, Chromium Palmer wrote: > Does this refer to the "Server-side characteristics" code below? > > If there is more work to be done, file a bug for it and include a > crbug.com/12345 link in the TODO comment. Done. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:84: base::TimeDelta SSLErrorClassification::TimePassedSinceExpiry() const{ On 2014/07/11 17:24:17, Chromium Palmer wrote: > "const {" here and on line 89. Done. https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/376663002/diff/390001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:16: const::net::X509Certificate& cert); On 2014/07/11 17:24:18, Chromium Palmer wrote: > Oops: No "::" between "const" and "net::". :) Done. I am surprised it compiled without any error.
lgtm
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/376663002/410001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/32384)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/376663002/430001
Message was sent while issue was closed.
Change committed as 282878 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
