Improve NetworkStateHandler API

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 7b0e0d9e0e084163753335c92e0a0d68995cf95d..51999333e6e1326408b0494243f242921287c301 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -108,7 +108,7 @@ const NetworkState* NetworkStateHandler::GetNetworkState(
   return GetModifiableNetworkState(service_path);
 }
 
-const NetworkState* NetworkStateHandler::ActiveNetwork() const {
+const NetworkState* NetworkStateHandler::DefaultNetwork() const {
   if (network_list_.empty())
     return NULL;
   const NetworkState* network = network_list_.front()->AsNetworkState();
@@ -126,7 +126,7 @@ const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
     DCHECK(network);
     if (!network->IsConnectedState())
       break;  // Connected networks are listed first.
-    if (network->type() == type)
+    if (network->MatchesType(type))
       return network;
   }
   return NULL;
@@ -142,10 +142,8 @@ const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
       continue;
     if (!network->IsConnectingState())
       break;  // Connected and connecting networks are listed first.
-    if (network->type() == type ||
-        (type.empty() && type != flimflam::kTypeEthernet)) {
+    if (network->MatchesType(type))
       return network;
-    }
   }
   return NULL;
 }
@@ -302,7 +300,7 @@ void NetworkStateHandler::UpdateManagedStateProperties(
     NetworkState* network = managed->AsNetworkState();
     DCHECK(network);
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                      NetworkServiceChanged(network));
+                      NetworkPropertyChanged(network));
   }
   network_event_log::AddEntry(
       kLogModule, "PropertiesReceived",
@@ -316,15 +314,18 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   NetworkState* network = GetModifiableNetworkState(service_path);
   if (!network)
     return;
-  if (ParseNetworkServiceProperty(network, key, value)) {
-    std::string detail = network->name() + "." + key;
-    std::string vstr;
-    if (value.GetAsString(&vstr))
-      detail += " = " + vstr;
-    network_event_log::AddEntry(kLogModule, "NetworkPropertyChanged", detail);
-    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                      NetworkServiceChanged(network));
-  }
+  std::string prev_state = network->state();
+  if (!ParseNetworkServiceProperty(network, key, value))
+    return;
+  if (key == flimflam::kStateProperty && network->state() == prev_state)

[pneubeck (no reviews), 2012/12/19 15:22:38]

Why is this required? The return value of ParseNetworkServiceProperty should
capture this case already.

[stevenjb, 2012/12/19 18:01:07]

This logic is now in ParseNetworkServiceProperty, but "parse" returns true if it
was parsed, regardless of whether or not it actually changed.

[pneubeck (no reviews), 2012/12/19 19:40:06]

Yeah, I was confused by the line
  if (!network->PropertyChanged(key, value))
which really reads like:
  if not the property |key| changed

+    return;  // Ignore noop state updates
+  std::string detail = network->name() + "." + key;
+  std::string vstr;
+  if (value.GetAsString(&vstr))
+    detail += " = " + vstr;
+  network_event_log::AddEntry(kLogModule, "NetworkPropertyChanged", detail);
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                    NetworkPropertyChanged(network));
 }
 
 void NetworkStateHandler::UpdateNetworkServiceIPAddress(
@@ -338,7 +339,7 @@ void NetworkStateHandler::UpdateNetworkServiceIPAddress(
   network->set_ip_address(ip_address);
   FOR_EACH_OBSERVER(
       NetworkStateHandlerObserver, observers_,
-      NetworkServiceChanged(network));
+      NetworkPropertyChanged(network));
 }
 
 void NetworkStateHandler::ManagerPropertyChanged() {
@@ -357,19 +358,7 @@ void NetworkStateHandler::ManagedStateListChanged(
         StringPrintf("Size: %"PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged(network_list));
-    // Update the active network and notify observers if it has changed.
-    NetworkState* new_active_network =
-        network_list_.empty() ? NULL : network_list_.front()->AsNetworkState();
-    std::string new_active_network_path;
-    if (new_active_network)
-      new_active_network_path = new_active_network->path();
-    if (new_active_network_path != active_network_path_) {
-      network_event_log::AddEntry(
-          kLogModule, "ActiveNetworkChanged", new_active_network_path);
-      active_network_path_ = new_active_network_path;
-      FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                        ActiveNetworkChanged(new_active_network));
-    }
+    CheckDefaultNetworkChanged();
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     network_event_log::AddEntry(
         kLogModule, "DeviceListChanged",
@@ -432,12 +421,37 @@ bool NetworkStateHandler::ParseNetworkServiceProperty(
   DCHECK(network);
   if (!network->PropertyChanged(key, value))
     return false;
-  if (network->path() == active_network_path_ &&
-      key == flimflam::kStateProperty) {
+  // Signal observers when the State changes.
+  if (key == flimflam::kStateProperty) {
+    std::string desc = StringPrintf(
+        "%s: %s", network->path().c_str(), network->state().c_str());
+    network_event_log::AddEntry(
+        kLogModule, "NetworkStateChanged", desc);
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                      ActiveNetworkStateChanged(network));
+                      NetworkStateChanged(network));
+    bool signaled_observers = CheckDefaultNetworkChanged();
+    if (!signaled_observers && network->path() == default_network_path_) {
+      // This was already the default network; signal observers.
+      FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                        DefaultNetworkChanged(network));
+    }
   }
   return true;
 }
 
+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;
+  network_event_log::AddEntry(
+      kLogModule, "DefaultNetworkChanged", new_default_network_path);
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                    DefaultNetworkChanged(new_default_network));
+  return true;
+}
+
 }  // namespace chromeos