Fixed current OOBE captive portal metrics.

chrome/browser/chromeos/net/network_portal_detector_impl.cc

Index: chrome/browser/chromeos/net/network_portal_detector_impl.cc
diff --git a/chrome/browser/chromeos/net/network_portal_detector_impl.cc b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
index b92dfe7633144f01997bb6c1ed44f64d797691bc..a1e9f09473c284d40dd02e2c217ee7e1480c6372 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_impl.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
@@ -39,31 +39,25 @@ const int kProxyChangeDelaySec = 1;
 // Delay between consecutive portal checks for a network in lazy mode.
 const int kLazyCheckIntervalSec = 5;
 
-const char kCaptivePortalStatusUnknown[] = "Unknown";
-const char kCaptivePortalStatusOffline[] = "Offline";
-const char kCaptivePortalStatusOnline[]  = "Online";
-const char kCaptivePortalStatusPortal[]  = "Portal";
-const char kCaptivePortalStatusProxyAuthRequired[] =
-    "Proxy authentication required";
-const char kCaptivePortalStatusUnrecognized[] = "Unrecognized";
-
-std::string CaptivePortalStatusString(
-    NetworkPortalDetectorImpl::CaptivePortalStatus status) {
-  switch (status) {
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_UNKNOWN:
-      return kCaptivePortalStatusUnknown;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_OFFLINE:
-      return kCaptivePortalStatusOffline;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_ONLINE:
-      return kCaptivePortalStatusOnline;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_PORTAL:
-      return kCaptivePortalStatusPortal;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
-      return kCaptivePortalStatusProxyAuthRequired;
-    case NetworkPortalDetectorImpl::CAPTIVE_PORTAL_STATUS_COUNT:
-      NOTREACHED();
+void RecordDiscrepancyWithShill(
+    const NetworkState* network,
+    const NetworkPortalDetector::CaptivePortalStatus status) {
+  if (network->connection_state() == shill::kStateOnline) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_Online",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStatePortal) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_RestrictedPool",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  } else if (network->connection_state() == shill::kStateOffline) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "CaptivePortal.OOBE.DiscrepancyWithShill_Offline",
+        status,
+        NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
   }
-  return kCaptivePortalStatusUnrecognized;
 }
 
 }  // namespace
@@ -452,11 +446,6 @@ bool NetworkPortalDetectorImpl::IsCheckingForPortal() const {
 void NetworkPortalDetectorImpl::SetCaptivePortalState(
     const NetworkState* network,
     const CaptivePortalState& state) {
-  if (!detection_start_time_.is_null()) {
-    UMA_HISTOGRAM_TIMES("CaptivePortal.OOBE.DetectionDuration",
-                        GetCurrentTimeTicks() - detection_start_time_);
-  }
-
   if (!network) {
     NotifyPortalDetectionCompleted(network, state);
     return;
@@ -472,6 +461,15 @@ void NetworkPortalDetectorImpl::SetCaptivePortalState(
             << "id=" << network->guid() << ", "
             << "status=" << CaptivePortalStatusString(state.status) << ", "
             << "response_code=" << state.response_code;
+
+    // Record detection duration iff detection result differs from the
+    // previous one for this network. The reason is to record only
+    // detection duration when network changes it's state, as we're
+    // interested only on distribution of detection duration, not

[Nikita (slow), 2014/01/09 13:48:56]

nit: update comment, all stats are reported only once per state changed.

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+    // distribution of detection duration multiplied by number of
+    // checks.
+    RecordDetectionStats(network, state.status);
+
     portal_state_map_[network->path()] = state;
   }
   NotifyPortalDetectionCompleted(network, state);
@@ -505,4 +503,44 @@ int NetworkPortalDetectorImpl::GetRequestTimeoutSec() const {
   return attempt_count_ * kBaseRequestTimeoutSec;
 }
 
+void NetworkPortalDetectorImpl::RecordDetectionStats(
+    const NetworkState* network,
+    CaptivePortalStatus status) {
+  // Don't record stats for offline state.
+  if (!network)
+    return;
+
+  if (!detection_start_time_.is_null()) {
+    UMA_HISTOGRAM_MEDIUM_TIMES("CaptivePortal.OOBE.DetectionDuration",
+                               GetCurrentTimeTicks() - detection_start_time_);
+  }
+  UMA_HISTOGRAM_ENUMERATION("CaptivePortal.OOBE.DetectionResult", status,

[Nikita (slow), 2014/01/09 13:48:56]

nit: status on a separate line.

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+                            NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT);
+  switch (status) {
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_UNKNOWN:
+      NOTREACHED();
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE:
+      if (network->connection_state() == shill::kStateOnline ||
+          network->connection_state() == shill::kStatePortal)
+        RecordDiscrepancyWithShill(network, status);

[Nikita (slow), 2014/01/09 13:48:56]

nit: {}

[ygorshenin1, 2014/01/09 14:59:35]

Done.

+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_ONLINE:
+      if (network->connection_state() != shill::kStateOnline)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL:
+      if (network->connection_state() != shill::kStatePortal)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
+      if (network->connection_state() != shill::kStateOnline)
+        RecordDiscrepancyWithShill(network, status);
+      break;
+    case NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_COUNT:
+      NOTREACHED();
+      break;
+  }
+}
+
 }  // namespace chromeos