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..a88819560484567adb2afa3c792dfcadcbe3dcdb 100644
--- a/chrome/browser/captive_portal/captive_portal_service.cc
+++ b/chrome/browser/captive_portal/captive_portal_service.cc
@@ -27,6 +27,22 @@ namespace captive_portal {
 
 namespace {
 
+// 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
+};
+
 // Records histograms relating to how often captive portal detection attempts
 // ended with |result| in a row, and for how long |result| was the last result
 // of a detection attempt.  Recorded both on quit and on a new Result.
@@ -69,6 +85,25 @@ void RecordRepeatHistograms(Result result,
   result_duration_histogram->AddTime(result_duration);
 }
 
+int GetHistogramEntryForDetectionResult(
+    const CaptivePortalDetector::Results& results) {
+  bool is_https = results.landing_url.SchemeIs("https");
+  switch (results.result) {
+    case RESULT_INTERNET_CONNECTED:
+      return DETECTION_RESULT_INTERNET_CONNECTED;
+    case RESULT_NO_RESPONSE:
+      return is_https
+          ? DETECTION_RESULT_NO_RESPONSE_HTTPS_LANDING_URL
+          : DETECTION_RESULT_NO_RESPONSE;
+    case RESULT_BEHIND_CAPTIVE_PORTAL:
+      return is_https
+          ? DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL_HTTPS_LANDING_URL
+          : DETECTION_RESULT_BEHIND_CAPTIVE_PORTAL;

[mmenke, 2014/03/26 19:11:27]

optional nit:  It's more common on chrome to put binary operators on the same
line as the first argument than the second.  I assume this applies to trinary
operators as well.

[meacer, 2014/03/26 19:22:48]

I looked this up when writing the code, and it seems to be a mix:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ref...
puts the operator in separate lines.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
puts them in the same line.

+  }
+  NOTREACHED();
+  return -1;
+}
+
 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 +245,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()) {