Introduce NetworkState::is_captive_portal()

chromeos/network/network_state.cc

Index: chromeos/network/network_state.cc
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index bd0f0d7b35910ca2d62047c53976b79ff979397e..6b8aff3295fa40ec0b9030ea12ea290a8334207a 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -29,6 +29,43 @@ bool ConvertListValueToStringVector(const base::ListValue& string_list,
   return true;
 }
 
+bool IsCaptivePortalState(const base::DictionaryValue& properties, bool log) {
+  std::string state;
+  properties.GetStringWithoutPathExpansion(shill::kStateProperty, &state);
+  if (state == shill::kStatePortal)

[pneubeck (no reviews), 2015/01/24 10:06:56]

shouldn't this be negated?

if (state != kStatePortal)
  return false;

[stevenjb, 2015/01/26 20:16:30]

Yes, thanks. Too many balls in the air.

+    return false;
+  std::string portal_detection_phase, portal_detection_status;
+  if (!properties.GetStringWithoutPathExpansion(
+          shill::kPortalDetectionFailedPhaseProperty,
+          &portal_detection_phase) ||
+      !properties.GetStringWithoutPathExpansion(
+          shill::kPortalDetectionFailedStatusProperty,
+          &portal_detection_status)) {
+    // If Shill (or a stub) has not set PortalDetectionFailedStatus
+    // or PortalDetectionFailedPhase, assume we are in captive portal state.
+    return true;
+  }
+  bool is_captive_portal =

[pneubeck (no reviews), 2015/01/24 10:06:56]

please add a comment what the rational of this is, e.g.:

"Shill reports in which phase it reached conclusion that it believes the device
behind a captive portal. We want to rely only on incorrect content being
returned and ignore other reasons."

[stevenjb, 2015/01/26 20:16:29]

Done.

+      portal_detection_status == shill::kPortalDetectionStatusFailure &&
+      portal_detection_phase == shill::kPortalDetectionPhaseContent;
+  if (!log)
+    return is_captive_portal;
+
+  std::string name;
+  properties.GetStringWithoutPathExpansion(shill::kNameProperty, &name);
+  if (name.empty())
+    properties.GetStringWithoutPathExpansion(shill::kSSIDProperty, &name);
+  if (portal_detection_status != shill::kPortalDetectionStatusFailure ||

[pneubeck (no reviews), 2015/01/24 10:06:56]

nit: condition can be replaced by !is_captive_portal.

[stevenjb, 2015/01/26 20:16:29]

Ditto, thanks, done.

+      portal_detection_phase != shill::kPortalDetectionPhaseContent) {
+    NET_LOG(EVENT) << "State is 'portal' but not in captive portal state:"
+                   << " name=" << name << " status=" << portal_detection_status
+                   << " phase=" << portal_detection_phase;
+  } else {
+    NET_LOG(EVENT) << "Network is in captive portal state: " << name;
+  }
+  return is_captive_portal;
+}
+
 }  // namespace
 
 namespace chromeos {
@@ -36,8 +73,9 @@ namespace chromeos {
 NetworkState::NetworkState(const std::string& path)
     : ManagedState(MANAGED_TYPE_NETWORK, path),
       visible_(false),
-      connectable_(false),
       prefix_length_(0),
+      connectable_(false),
+      is_captive_portal_(false),
       signal_strength_(0),
       cellular_out_of_credits_(false) {
 }
@@ -141,9 +179,13 @@ bool NetworkState::InitialPropertiesReceived(
   // SignalStrength > 0.
   if ((type() == shill::kTypeWifi || type() == shill::kTypeWimax) &&
       visible() && signal_strength_ <= 0) {
-      signal_strength_ = 1;
+    signal_strength_ = 1;
   }
 
+  // Any change to connection state will trigger a complete property update,
+  // so we update is_captive_portal_ here.
+  is_captive_portal_ = IsCaptivePortalState(properties, true /* log */);
+
   // Ensure that the network has a valid name.
   return UpdateName(properties);
 }
@@ -182,9 +224,8 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
 
   // Mobile properties
   if (NetworkTypePattern::Mobile().MatchesType(type())) {
-    dictionary->SetStringWithoutPathExpansion(
-        shill::kNetworkTechnologyProperty,
-        network_technology());
+    dictionary->SetStringWithoutPathExpansion(shill::kNetworkTechnologyProperty,
+                                              network_technology());
     dictionary->SetStringWithoutPathExpansion(shill::kActivationStateProperty,
                                               activation_state());
     dictionary->SetStringWithoutPathExpansion(shill::kRoamingStateProperty,
@@ -196,8 +237,8 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
 
 void NetworkState::IPConfigPropertiesChanged(
     const base::DictionaryValue& properties) {
-  for (base::DictionaryValue::Iterator iter(properties);
-       !iter.IsAtEnd(); iter.Advance()) {
+  for (base::DictionaryValue::Iterator iter(properties); !iter.IsAtEnd();
+       iter.Advance()) {
     std::string key = iter.key();
     const base::Value& value = iter.value();
 
@@ -262,7 +303,7 @@ bool NetworkState::IsInProfile() const {
 
 bool NetworkState::IsPrivate() const {
   return !profile_path_.empty() &&
-      profile_path_ != NetworkProfileHandler::GetSharedProfilePath();
+         profile_path_ != NetworkProfileHandler::GetSharedProfilePath();
 }
 
 std::string NetworkState::GetDnsServersAsString() const {
@@ -281,7 +322,7 @@ std::string NetworkState::GetNetmask() const {
 
 std::string NetworkState::GetSpecifier() const {
   if (!update_received()) {
-    NET_LOG(ERROR) << "GetSpecifier called before update: " <<  path();
+    NET_LOG(ERROR) << "GetSpecifier called before update: " << path();
     return std::string();
   }
   if (type() == shill::kTypeWifi)
@@ -320,6 +361,12 @@ bool NetworkState::StateIsConnecting(const std::string& connection_state) {
 }
 
 // static
+bool NetworkState::NetworkStateIsCaptivePortal(
+    const base::DictionaryValue& shill_properties) {
+  return IsCaptivePortalState(shill_properties, false /* log */);
+}
+
+// static
 bool NetworkState::ErrorIsValid(const std::string& error) {
   // Shill uses "Unknown" to indicate an unset or cleared error state.
   return !error.empty() && error != kErrorUnknown;