Add additional UMA stats for remembering certificate decisions.

chrome/browser/ssl/ssl_blocking_page.cc

Index: chrome/browser/ssl/ssl_blocking_page.cc
diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc
index d9cb0020a77f6c680d832d172f32f0305bf66526..eabc8ab8eff2e64c436f68d57ec916d03fe703e7 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -102,24 +102,50 @@ enum SSLBlockingPageEvent {
   UNUSED_BLOCKING_PAGE_EVENT,
 };
 
+enum SSLIsExpiredAndDecision {

[felt, 2014/08/08 00:17:06]

sort of a weird name for an enum, the "Is" makes it sound like it's a bool.
SSLDesicionAfterExpiration? SSLExpirationAndDecision?

[felt, 2014/08/08 00:17:06]

Add a note warning people that this is a UMA histogram, e.g.,
// Events for UMA. Do not reorder or change!

[jww, 2014/08/08 15:55:45]

Done.

[jww, 2014/08/08 15:55:45]

Done.

+  EXPIRED_AND_PROCEED,
+  EXPIRED_AND_DO_NOT_PROCEED,
+  NOT_EXPIRED_AND_PROCEED,
+  NOT_EXPIRED_AND_DO_NOT_PROCEED,
+  END_OF_SSL_IS_EXPIRED_AND_DECISION,
+};
+
 void RecordSSLBlockingPageEventStats(SSLBlockingPageEvent event) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl",
                             event,
                             UNUSED_BLOCKING_PAGE_EVENT);
 }
 
-void RecordSSLBlockingPageDetailedStats(
-    bool proceed,
-    int cert_error,
-    bool overridable,
-    bool internal,
-    int num_visits,
-    bool captive_portal_detection_enabled,
-    bool captive_portal_probe_completed,
-    bool captive_portal_no_response,
-    bool captive_portal_detected) {
+void RecordSSLIsExpiredPageEventState(bool expired_but_previously_allowed,

[felt, 2014/08/08 00:17:06]

it seems like this will count all non-overridable ones as
NOT_EXPRED_AND_DO_NOT_PROCEED? perhaps you ought to handle non-overridable ones
with a separate entry (or entries), or don't include them in the histogram at
all.

[jww, 2014/08/08 15:55:45]

Good call. I'll split them into
"interstitial.ssl.expired_and_decision.overridable" and
"interstitial.ssl.expired_and_decision.nonoverridable". I'd like to see if
there's a difference between the two.

+                                      bool proceed) {
+  SSLIsExpiredAndDecision event;
+  if (expired_but_previously_allowed && proceed)
+    event = EXPIRED_AND_PROCEED;
+  else if (expired_but_previously_allowed && !proceed)
+    event = EXPIRED_AND_DO_NOT_PROCEED;
+  else if (!expired_but_previously_allowed && proceed)
+    event = NOT_EXPIRED_AND_PROCEED;
+  else
+    event = NOT_EXPIRED_AND_DO_NOT_PROCEED;
+
+  UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.is_expired_and_decision",
+                            event,
+                            END_OF_SSL_IS_EXPIRED_AND_DECISION);
+}
+
+void RecordSSLBlockingPageDetailedStats(bool proceed,
+                                        int cert_error,
+                                        bool overridable,
+                                        bool internal,
+                                        int num_visits,
+                                        bool captive_portal_detection_enabled,
+                                        bool captive_portal_probe_completed,
+                                        bool captive_portal_no_response,
+                                        bool captive_portal_detected,
+                                        bool expired_but_previously_allowed) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_type",
       SSLErrorInfo::NetErrorToErrorType(cert_error), SSLErrorInfo::END_OF_ENUM);
+  RecordSSLIsExpiredPageEventState(expired_but_previously_allowed, proceed);
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
   if (captive_portal_detection_enabled)
     RecordSSLBlockingPageEventStats(
@@ -269,14 +295,14 @@ void LaunchDateAndTimeSettings() {
 
 // Note that we always create a navigation entry with SSL errors.
 // No error happening loading a sub-resource triggers an interstitial so far.
-SSLBlockingPage::SSLBlockingPage(
-    content::WebContents* web_contents,
-    int cert_error,
-    const net::SSLInfo& ssl_info,
-    const GURL& request_url,
-    bool overridable,
-    bool strict_enforcement,
-    const base::Callback<void(bool)>& callback)
+SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents,
+                                 int cert_error,
+                                 const net::SSLInfo& ssl_info,
+                                 const GURL& request_url,
+                                 bool overridable,
+                                 bool strict_enforcement,
+                                 bool expired_but_previously_allowed,
+                                 const base::Callback<void(bool)>& callback)
     : callback_(callback),
       web_contents_(web_contents),
       cert_error_(cert_error),
@@ -290,7 +316,8 @@ SSLBlockingPage::SSLBlockingPage(
       captive_portal_detection_enabled_(false),
       captive_portal_probe_completed_(false),
       captive_portal_no_response_(false),
-      captive_portal_detected_(false) {
+      captive_portal_detected_(false),
+      expired_but_previously_allowed_(expired_but_previously_allowed) {
   Profile* profile = Profile::FromBrowserContext(
       web_contents->GetBrowserContext());
   // For UMA stats.
@@ -341,7 +368,8 @@ SSLBlockingPage::~SSLBlockingPage() {
                                        captive_portal_detection_enabled_,
                                        captive_portal_probe_completed_,
                                        captive_portal_no_response_,
-                                       captive_portal_detected_);
+                                       captive_portal_detected_,
+                                       expired_but_previously_allowed_);
     // The page is closed without the user having chosen what to do, default to
     // deny.
     NotifyDenyCertificate();
@@ -523,7 +551,8 @@ void SSLBlockingPage::OnProceed() {
                                      captive_portal_detection_enabled_,
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     captive_portal_detected_,
+                                     expired_but_previously_allowed_);
   // Accepting the certificate resumes the loading of the page.
   NotifyAllowCertificate();
 }
@@ -537,7 +566,8 @@ void SSLBlockingPage::OnDontProceed() {
                                      captive_portal_detection_enabled_,
                                      captive_portal_probe_completed_,
                                      captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     captive_portal_detected_,
+                                     expired_but_previously_allowed_);
   NotifyDenyCertificate();
 }