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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 
 #include "base/build_time.h"
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
@@ skipping 98 matching lines @@
   DONT_PROCEED_NAME,
   DONT_PROCEED_DATE,
   DONT_PROCEED_AUTHORITY,
   MORE,
   SHOW_UNDERSTAND,  // Used by the summer 2013 Finch trial. Deprecated.
   SHOW_INTERNAL_HOSTNAME,
   PROCEED_INTERNAL_HOSTNAME,
   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,
   CAPTIVE_PORTAL_NO_RESPONSE_OVERRIDABLE,
   CAPTIVE_PORTAL_DETECTED,
   CAPTIVE_PORTAL_DETECTED_OVERRIDABLE,
   UNUSED_BLOCKING_PAGE_EVENT,
 };
 
 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) {
+    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);
     }
     // Overridable is false if the user didn't have any option except to turn
     // back. If that's the case, don't record some of the metrics.
     return;
   }
   if (num_visits == 0)
     RecordSSLBlockingPageEventStats(SHOW_NEW_SITE);
@@ skipping 74 matching lines @@
     HistoryService* history_service = HistoryServiceFactory::GetForProfile(
         profile, Profile::EXPLICIT_ACCESS);
     if (history_service) {
       history_service->GetVisibleVisitCountToHost(
           request_url_,
           base::Bind(&SSLBlockingPage::OnGotHistoryCount,
                      base::Unretained(this)),
           &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 =
       CaptivePortalServiceFactory::GetForProfile(profile);
   captive_portal_detection_enabled_ = captive_portal_service ->enabled();
   captive_portal_service ->DetectCaptivePortal();
   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(
       web_contents_, true, request_url, this);
   interstitial_page_->Show();
 }
 
 SSLBlockingPage::~SSLBlockingPage() {
   if (!callback_.is_null()) {
     RecordSSLBlockingPageDetailedStats(false,
                                        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();
   }
 }
 
 std::string SSLBlockingPage::GetHTMLContents() {
   if (trial_condition_ == kCondV1 || trial_condition_ == kCondV1LayoutV2Text)
     return GetHTMLContentsV1();
   return GetHTMLContentsV2();
@@ skipping 320 matching lines @@
   Profile* profile = Profile::FromBrowserContext(
       web_contents_->GetBrowserContext());
   renderer_preferences_util::UpdateFromSystemSettings(prefs, profile);
 }
 
 void SSLBlockingPage::OnProceed() {
   RecordSSLBlockingPageDetailedStats(true,
                                      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();
 }
 
 void SSLBlockingPage::OnDontProceed() {
   RecordSSLBlockingPageDetailedStats(false,
                                      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();
 }
 
 void SSLBlockingPage::NotifyDenyCertificate() {
   // It's possible that callback_ may not exist if the user clicks "Proceed"
   // followed by pressing the back button before the interstitial is hidden.
   // In that case the certificate will still be treated as allowed.
   if (callback_.is_null())
     return;
 
@@ skipping 55 matching lines @@
     // sure we don't clear the captive portal flag, since the interstitial was
     // potentially caused by the captive portal.
     captive_portal_detected_ = captive_portal_detected_ ||
         (results->result == captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL);
     // Also keep track of non-HTTP portals and error cases.
     captive_portal_no_response_ = captive_portal_no_response_ ||
         (results->result == captive_portal::RESULT_NO_RESPONSE);
   }
 #endif
 }