Chrome OS: Use Manager.DefaultService for Default Network

chromeos/network/network_state_handler.cc

Index: chromeos/network/network_state_handler.cc
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index f3a93395d06a1878bfa3fc881f2eb9aa3cbb2714..a77f42387241e288f30627b2475f7cdc7c37a1b8 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -180,13 +180,9 @@ const NetworkState* NetworkStateHandler::GetNetworkState(
 }
 
 const NetworkState* NetworkStateHandler::DefaultNetwork() const {
-  if (network_list_.empty())
+  if (default_network_path_.empty())

[mukesh agrawal, 2014/02/25 02:08:41]

Not sure about this check. When there is no connected network, shill will send a
path of "/". (It can't send an empty string, because the value is encoded as a
D-Bus path type, and path types aren't allowed to be empty.)

[stevenjb, 2014/02/25 21:31:39]

Hmm, that's unfortunate, but I can add a test for that where we set
default_network_path_. The code will work as-is since GetNetworkState() will
return NULL if the path doesn't have an associated service, but for debugging,
etc, it would be better to use an empty string for no default network.

     return NULL;
-  const NetworkState* network = network_list_.front()->AsNetworkState();
-  DCHECK(network);
-  if (!network->update_received() || !network->IsConnectedState())
-    return NULL;
-  return network;
+  return GetNetworkState(default_network_path_);
 }
 
 const FavoriteState* NetworkStateHandler::DefaultFavoriteNetwork() const {
@@ -538,7 +534,8 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
     // Signal connection state changed after all properties have been updated.
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
-    NetworkPropertiesUpdated(network);
+    NET_LOG_EVENT("NetworkPropertiesUpdated", GetManagedStateLogName(network));
+    NotifyNetworkPropertiesUpdated(network);
   }
 }
 
@@ -580,10 +577,13 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
     if (key != shill::kSignalStrengthProperty &&
         key != shill::kWifiFrequencyListProperty &&
         (key != shill::kDeviceProperty || value_str != "/")) {
+      std::string log_event = "NetworkPropertyUpdated";
       // Trigger a default network update for interesting changes only.
-      if (network->path() == default_network_path_)
-        OnDefaultNetworkChanged();
-      // Log interesting event.
+      if (network->path() == default_network_path_) {
+        NotifyDefaultNetworkChanged(network);
+        log_event = "Default" + log_event;
+      }
+      // Log event.
       std::string detail = network->name() + "." + key;
       detail += " = " + network_event_log::ValueAsString(value);
       network_event_log::LogLevel log_level;
@@ -592,12 +592,12 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
       } else {
         log_level = network_event_log::LOG_LEVEL_EVENT;
       }
-      NET_LOG_LEVEL(log_level, "NetworkPropertyUpdated", detail);
+      NET_LOG_LEVEL(log_level, log_event, detail);
     }
   }
 
   // All property updates signal 'NetworkPropertiesUpdated'.
-  NetworkPropertiesUpdated(network);
+  NotifyNetworkPropertiesUpdated(network);
 
   // If added to a Profile, request a full update so that a FavoriteState
   // gets created.
@@ -657,9 +657,6 @@ void NetworkStateHandler::ManagedStateListChanged(
                   base::StringPrintf("Size:%" PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged());
-    // The list order may have changed, so check if the default network changed.
-    if (CheckDefaultNetworkChanged())
-      OnDefaultNetworkChanged();
     // Update UMA stats.
     UMA_HISTOGRAM_COUNTS_100("Networks.Visible", network_list_.size());
   } else if (type == ManagedState::MANAGED_TYPE_FAVORITE) {
@@ -689,6 +686,28 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
+void NetworkStateHandler::DefaultNetworkServiceChanged(
+    const std::string& service_path) {
+  default_network_path_ = service_path;
+  NET_LOG_EVENT("DefaultNetworkServiceChanged", service_path);
+  const NetworkState* network = NULL;
+  if (!service_path.empty()) {

[mukesh agrawal, 2014/02/25 02:08:41]

Same here (might need to check equality against '/' instead of .empty()).

[stevenjb, 2014/02/25 21:31:39]

I will add the check for "/" where we set default_network_path_.

+    network = GetNetworkState(service_path);
+    if (!network) {
+      // If NetworkState is not available yet, do not notify observers here,
+      // they will be notified when the state is received.
+      NET_LOG_DEBUG("Default NetworkState not available", service_path);
+      return;
+    }
+  }
+  if (network && !network->IsConnectedState()) {
+    NET_LOG_ERROR(
+        "DefaultNetwork is not connected: " + network->connection_state(),
+        network->path());
+  }
+  NotifyDefaultNetworkChanged(network);
+}
+
 //------------------------------------------------------------------------------
 // Private methods
 
@@ -745,35 +764,30 @@ NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
 void NetworkStateHandler::OnNetworkConnectionStateChanged(
     NetworkState* network) {
   DCHECK(network);
-  NET_LOG_EVENT("NetworkConnectionStateChanged", base::StringPrintf(
-      "%s:%s", GetManagedStateLogName(network).c_str(),
-      network->connection_state().c_str()));
+  std::string event = "NetworkConnectionStateChanged";
+  if (network->path() == default_network_path_) {
+    event = "Default" + event;
+    if (!network->IsConnectedState()) {
+      NET_LOG_ERROR(
+          "DefaultNetwork is not connected: " + network->connection_state(),
+          network->path());
+    }
+  }
+  NET_LOG_EVENT(event + ": " + network->connection_state(),
+                GetManagedStateLogName(network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkConnectionStateChanged(network));
-  if (CheckDefaultNetworkChanged() || network->path() == default_network_path_)
-    OnDefaultNetworkChanged();
-}
-
-bool NetworkStateHandler::CheckDefaultNetworkChanged() {
-  std::string new_default_network_path;
-  const NetworkState* new_default_network = DefaultNetwork();
-  if (new_default_network)
-    new_default_network_path = new_default_network->path();
-  if (new_default_network_path == default_network_path_)
-    return false;
-  default_network_path_ = new_default_network_path;
-  return true;
+  if (network->path() == default_network_path_)
+    NotifyDefaultNetworkChanged(network);
 }
 
-void NetworkStateHandler::OnDefaultNetworkChanged() {
-  const NetworkState* default_network = DefaultNetwork();
-  NET_LOG_EVENT("DefaultNetworkChanged",
-                GetManagedStateLogName(default_network));
+void NetworkStateHandler::NotifyDefaultNetworkChanged(
+    const NetworkState* default_network) {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DefaultNetworkChanged(default_network));
 }
 
-void NetworkStateHandler::NetworkPropertiesUpdated(
+void NetworkStateHandler::NotifyNetworkPropertiesUpdated(
     const NetworkState* network) {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkPropertiesUpdated(network));