Refactor the captive portal code to move from the ssl_blocking_page class to the ssl_error_classific

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 ca97c2b60a0a90009235bd3716fc0b3b657f2126..117d54b252d9d9ed1a95ae6f0de2726deda7abc2 100644
--- a/chrome/browser/ssl/ssl_blocking_page.cc
+++ b/chrome/browser/ssl/ssl_blocking_page.cc
@@ -116,8 +116,8 @@ enum SSLBlockingPageEvent {
   SHOW_NEW_SITE,
   PROCEED_NEW_SITE,
   PROCEED_MANUAL_NONOVERRIDABLE,
-  CAPTIVE_PORTAL_DETECTION_ENABLED,
-  CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE,
+  CAPTIVE_PORTAL_DETECTION_ENABLED, // Captive Portal errors moved to

[felt, 2014/07/18 23:53:42]

Can you rename them like:

DEPRECATED_CAPTIVE_PORTAL_DETECTION_ENABLED,

[meacer, 2014/07/19 00:06:45]

I think it makes sense to put captive portal stuff into a new enum.

[radhikabhar, 2014/07/21 16:05:11]

I have already created a new enum in the ssl_error_classification and in the
histogram.xml - should I just delete the captive portal stuff from this file as
well as remove it from the enum in the histogram.xml file?

[radhikabhar, 2014/07/21 16:05:11]

Done.

[felt, 2014/07/21 18:14:45]

I think you have done it the correct way now. Msutafa might have missed that you
created the new enum. We can't delete the captive portal entries in the enum or
else it will screw up the UMA dashboards. All we can do is mark them as
deprecated (which it looks like you've already done).

+  CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE, // ssl_error_classification.
   CAPTIVE_PORTAL_PROBE_COMPLETED,
   CAPTIVE_PORTAL_PROBE_COMPLETED_OVERRIDABLE,
   CAPTIVE_PORTAL_NO_RESPONSE,
@@ -138,36 +138,9 @@ void RecordSSLBlockingPageDetailedStats(
     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) {
+    int num_visits) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.ssl_error_type",
       SSLErrorInfo::NetErrorToErrorType(cert_error), SSLErrorInfo::END_OF_ENUM);
-#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
-  if (captive_portal_detection_enabled)
-    RecordSSLBlockingPageEventStats(
-        overridable ?
-        CAPTIVE_PORTAL_DETECTION_ENABLED_OVERRIDABLE :
-        CAPTIVE_PORTAL_DETECTION_ENABLED);
-  if (captive_portal_probe_completed)
-    RecordSSLBlockingPageEventStats(
-        overridable ?
-        CAPTIVE_PORTAL_PROBE_COMPLETED_OVERRIDABLE :
-        CAPTIVE_PORTAL_PROBE_COMPLETED);
-  // Log only one of portal detected and no response results.
-  if (captive_portal_detected)
-    RecordSSLBlockingPageEventStats(
-        overridable ?
-        CAPTIVE_PORTAL_DETECTED_OVERRIDABLE :
-        CAPTIVE_PORTAL_DETECTED);
-  else if (captive_portal_no_response)
-    RecordSSLBlockingPageEventStats(
-        overridable ?
-        CAPTIVE_PORTAL_NO_RESPONSE_OVERRIDABLE :
-        CAPTIVE_PORTAL_NO_RESPONSE);
-#endif
   if (!overridable) {
     if (proceed) {
       RecordSSLBlockingPageEventStats(PROCEED_MANUAL_NONOVERRIDABLE);
@@ -262,11 +235,15 @@ SSLBlockingPage::SSLBlockingPage(
           &request_tracker_);
     }
   }
+
+  SSLErrorClassification ssl_error_classification(
+      *web_contents_,
+      base::Time::NowFromSystemTime(),
+      *ssl_info_.cert.get());
   if (SSLErrorInfo::NetErrorToErrorType(cert_error_) ==
-      SSLErrorInfo::CERT_DATE_INVALID) {
-    SSLErrorClassification::RecordUMAStatistics(overridable_ &&
-                                                !strict_enforcement_);
-  }
+      SSLErrorInfo::CERT_DATE_INVALID)

[felt, 2014/07/18 23:53:42]

not a single line, so it needs { }
(if i missed this in the other CL, it needs it there too)

[radhikabhar, 2014/07/21 16:05:11]

Done.

+    ssl_error_classification.RecordUMAStatistics(
+        overridable_ && !strict_enforcement_);
 
 #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
   CaptivePortalService* captive_portal_service =
@@ -276,6 +253,8 @@ SSLBlockingPage::SSLBlockingPage(
   registrar_.Add(this,
                  chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
                  content::Source<Profile>(profile));
+  ssl_error_classification.RecordUMAStatisticsCaptivePortals(
+      overridable_ && !strict_enforcement_);
 #endif
 
   interstitial_page_ = InterstitialPage::Create(
@@ -289,11 +268,7 @@ SSLBlockingPage::~SSLBlockingPage() {
                                        cert_error_,
                                        overridable_ && !strict_enforcement_,
                                        internal_,
-                                       num_visits_,
-                                       captive_portal_detection_enabled_,
-                                       captive_portal_probe_completed_,
-                                       captive_portal_no_response_,
-                                       captive_portal_detected_);
+                                       num_visits_);
     // The page is closed without the user having chosen what to do, default to
     // deny.
     NotifyDenyCertificate();
@@ -634,11 +609,7 @@ void SSLBlockingPage::OnProceed() {
                                      cert_error_,
                                      overridable_ && !strict_enforcement_,
                                      internal_,
-                                     num_visits_,
-                                     captive_portal_detection_enabled_,
-                                     captive_portal_probe_completed_,
-                                     captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     num_visits_);
   // Accepting the certificate resumes the loading of the page.
   NotifyAllowCertificate();
 }
@@ -648,11 +619,7 @@ void SSLBlockingPage::OnDontProceed() {
                                      cert_error_,
                                      overridable_ && !strict_enforcement_,
                                      internal_,
-                                     num_visits_,
-                                     captive_portal_detection_enabled_,
-                                     captive_portal_probe_completed_,
-                                     captive_portal_no_response_,
-                                     captive_portal_detected_);
+                                     num_visits_);
   NotifyDenyCertificate();
 }