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..5069c24b984a7462a4b34500ae39f1c5a388813f 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -180,13 +180,7 @@ const NetworkState* NetworkStateHandler::GetNetworkState(
 }
 
 const NetworkState* NetworkStateHandler::DefaultNetwork() const {

[pneubeck (no reviews), 2014/02/24 09:22:58]

you dropped the check for IsConnectedState.
Considering that Mukesh is adapting Shill's side, we might still add an error
log if it's not the case (e.g. because the State wasn't updated quickly enough).

[stevenjb, 2014/02/24 23:59:42]

So, this is kind of tricky. I want to keep the return value for this consistent
with what gets passed to DefaultNetworkChanged(). So, instead of checking
connection state here, I added logging in DefaultNetworkServiceChanged() and
OnNetworkConnectionStateChanged().

-  if (network_list_.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_);

[pneubeck (no reviews), 2014/02/24 09:22:58]

should early return on default_network_path_.empty()
(it works as it is, but we shouldn't on purpose search for incorrect paths)

[stevenjb, 2014/02/24 23:59:42]

Done.

 }
 
 const FavoriteState* NetworkStateHandler::DefaultFavoriteNetwork() const {
@@ -538,7 +532,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 +575,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 +590,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 +655,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 +684,15 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
+void NetworkStateHandler::DefaultNetworkServiceChanged(
+    const std::string& service_path) {
+  default_network_path_ = service_path;
+  const NetworkState* network = DefaultNetwork();
+  NET_LOG_EVENT("DefaultNetworkServiceChanged",
+                GetManagedStateLogName(network));
+  NotifyDefaultNetworkChanged(network);
+}
+
 //------------------------------------------------------------------------------
 // Private methods
 
@@ -745,35 +749,24 @@ 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;
+  NET_LOG_EVENT(event + ": " + network->connection_state(),
+                GetManagedStateLogName(network));

[pneubeck (no reviews), 2014/02/24 09:22:58]

just an idea: you could also add "(default)" to the result of
GetManagedStateLogName if the network is the default one.

[stevenjb, 2014/02/24 23:59:42]

Interesting thought. I'm not sure I want to lengthen every log event for the
network though (whereas calling attention to specific events that affect the NCN
is helpful).

   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkConnectionStateChanged(network));
-  if (CheckDefaultNetworkChanged() || network->path() == default_network_path_)
-    OnDefaultNetworkChanged();
+  if (network->path() == default_network_path_)
+    NotifyDefaultNetworkChanged(network);
 }
 
-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;
-}
-
-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));