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..dd6f568f25eab173d53f3b7b06c7344e70b99eb0 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())
     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,32 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
+void NetworkStateHandler::DefaultNetworkServiceChanged(
+    const std::string& service_path) {
+  const char* kEmptyServicePath = "/";

[pneubeck (no reviews), 2014/02/26 08:52:42]

Better add a short comment why we are doing this.

[stevenjb, 2014/02/26 18:04:47]

Done.

+  if (service_path == kEmptyServicePath)
+    default_network_path_.clear();
+  else
+    default_network_path_ = service_path;

[pneubeck (no reviews), 2014/02/26 08:52:42]

everything after this line should probably use default_network_path_ instead of
service_path.

[stevenjb, 2014/02/26 18:04:47]

Yes indeed, thanks.

+  NET_LOG_EVENT("DefaultNetworkServiceChanged", service_path);
+  const NetworkState* network = NULL;
+  if (!service_path.empty()) {

[gauravsh, 2014/02/26 00:44:32]

Shouldn't this check also be service_path == kEmptyServicePath

[stevenjb, 2014/02/26 18:04:47]

Ugh, thanks. We should use default_network_path_ below line 695 now.
Done.

+    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 +768,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));