Add logging for distinguishing between captive portals with HTTP and HTTPS login pages

chrome/browser/captive_portal/captive_portal_service.cc

Index: chrome/browser/captive_portal/captive_portal_service.cc
diff --git a/chrome/browser/captive_portal/captive_portal_service.cc b/chrome/browser/captive_portal/captive_portal_service.cc
index dc40d2a22682bf8807c9eb2e55e7f4f0b450a59b..1da7c718d1cf2d8260cb1533c760b04b191c9f9e 100644
--- a/chrome/browser/captive_portal/captive_portal_service.cc
+++ b/chrome/browser/captive_portal/captive_portal_service.cc
@@ -25,6 +25,22 @@
 
 namespace captive_portal {
 
+// Make sure this enum is in sync with CaptivePortalDetectionResult enum
+// in histograms.xml.
+enum CaptivePortalDetectionResult {
+  // There's a confirmed connection to the Internet.
+  DETECTION_RESULT_INTERNET_CONNECTED,
+  // Received a network or HTTP error, or a non-HTTP response.
+  DETECTION_RESULT_NO_RESPONSE,
+  // Encountered a captive portal with a non-HTTPS landing URL.
+  DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL,
+  // Received a network or HTTP error with an HTTPS landing URL.
+  DETECTION_RESULT_NO_RESPONSE_HTTPS_LANDING_URL,
+  // Encountered a captive portal with an HTTPS landing URL.
+  DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL_HTTPS_LANDING_URL,
+  DETECTION_RESULT_COUNT
+};

[mmenke, 2014/03/26 18:05:36]

This should be in the anonymous namespace.

[meacer, 2014/03/26 19:07:32]

Done.

+
 namespace {
 
 // Records histograms relating to how often captive portal detection attempts
@@ -69,6 +85,22 @@ void RecordRepeatHistograms(Result result,
   result_duration_histogram->AddTime(result_duration);
 }
 
+int GetHistogramEntryForDetectionResult(
+    const CaptivePortalDetector::Results& results) {
+  bool is_https = results.landing_url.SchemeIs("https");

[mmenke, 2014/03/26 18:05:36]

Should use a switch on results.result.  It's more code, but if you don't leave a
default case, compilation will fail if a new result is added - that's my main
concern here.

[meacer, 2014/03/26 19:07:32]

Done.

+  if (results.result == RESULT_NO_RESPONSE) {
+    return is_https
+        ? DETECTION_RESULT_NO_RESPONSE_HTTPS_LANDING_URL
+        : DETECTION_RESULT_NO_RESPONSE;
+  }
+  if (results.result == RESULT_BEHIND_CAPTIVE_PORTAL) {
+    return is_https
+        ? DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL_HTTPS_LANDING_URL
+        : DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL;
+  }
+  return DETECTION_RESULT_INTERNET_CONNECTED;
+}
+
 bool ShouldDeferToNativeCaptivePortalDetection() {
   // On Windows 8, defer to the native captive portal detection.  OSX Lion and
   // later also have captive portal detection, but experimentally, this code
@@ -210,8 +242,8 @@ void CaptivePortalService::OnPortalDetectionCompleted(
 
   // Record histograms.
   UMA_HISTOGRAM_ENUMERATION("CaptivePortal.DetectResult",
-                            result,
-                            RESULT_COUNT);
+                            GetHistogramEntryForDetectionResult(results),
+                            DETECTION_RESULT_COUNT);
 
   // If this isn't the first captive portal result, record stats.
   if (!last_check_time_.is_null()) {