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

chrome/browser/captive_portal/captive_portal_service.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/captive_portal/captive_portal_service.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 
 #if defined(OS_MACOSX)
 #include "base/mac/mac_util.h"
 #endif
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #endif
 
 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
 // 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.
 //
 // |repeat_count| may be 0 if there were no captive portal checks during
 // a session.
 //
 // |result_duration| is the time between when a captive portal check first
@@ skipping 24 matching lines @@
       base::Histogram::FactoryTimeGet(
           "CaptivePortal.ResultDuration." +
               CaptivePortalDetector::CaptivePortalResultToString(result),
           base::TimeDelta::FromSeconds(1),  // min
           base::TimeDelta::FromHours(1),  // max
           50,  // bucket_count
           base::Histogram::kUmaTargetedHistogramFlag);
   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
   // works in cases its does not.
   //
   // TODO(mmenke): Investigate how well Windows 8's captive portal detection
   // works.
 #if defined(OS_WIN)
   return base::win::GetVersion() >= base::win::VERSION_WIN8;
 #else
@@ skipping 121 matching lines @@
   DCHECK_EQ(STATE_CHECKING_FOR_PORTAL, state_);
   DCHECK(!TimerRunning());
   DCHECK(enabled_);
 
   Result result = results.result;
   const base::TimeDelta& retry_after_delta = results.retry_after_delta;
   base::TimeTicks now = GetCurrentTimeTicks();
 
   // 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()) {
     UMA_HISTOGRAM_LONG_TIMES("CaptivePortal.TimeBetweenChecks",
                              now - last_check_time_);
 
     if (last_detection_result_ != result) {
       // If the last result was different from the result of the latest test,
       // record histograms about the previous period over which the result was
       // the same.
@@ skipping 113 matching lines @@
 
 bool CaptivePortalService::DetectionInProgress() const {
   return state_ == STATE_CHECKING_FOR_PORTAL;
 }
 
 bool CaptivePortalService::TimerRunning() const {
   return check_captive_portal_timer_.IsRunning();
 }
 
 }  // namespace captive_portal