Improve NetworkStateHandler API

chromeos/network/network_state_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/network_state_handler.h"
 
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ skipping 90 matching lines @@
       return device->AsDeviceState();
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::GetNetworkState(
     const std::string& service_path) const {
   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();
   DCHECK(network);
   if (!network->IsConnectedState())
     return NULL;
   return network;
 }
 
 const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
     const std::string& type) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->IsConnectedState())
       break;  // Connected networks are listed first.
-    if (network->type() == type)
+    if (network->MatchesType(type))
       return network;
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
     const std::string& type) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (network->IsConnectedState())
       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;
 }
 
 std::string NetworkStateHandler::HardwareAddressForType(
     const std::string& type) const {
   std::string result;
   const NetworkState* network = ConnectedNetworkByType(type);
   if (network) {
     const DeviceState* device = GetDeviceState(network->device_path());
@@ skipping 123 matching lines @@
     const base::DictionaryValue& properties) {
   ManagedState* managed = GetModifiableManagedState(GetManagedList(type), path);
   if (!managed) {
     LOG(ERROR) << "GetPropertiesCallback: " << path << " Not found!";
     return;
   }
   bool network_property_changed = false;
   for (base::DictionaryValue::Iterator iter(properties);
        iter.HasNext(); iter.Advance()) {
     if (type == ManagedState::MANAGED_TYPE_NETWORK) {
       if (ParseNetworkServiceProperty(

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

Why are you interested here in if something was successfully parsed and not if
something actually changed?

That something changed implies that we parsed that value successfully. Thus it
would only reduce the amount of notifications.

[stevenjb, 2012/12/19 21:36:50]

That is misleading. I renamed network_property_changed to
network_property_updated and renamed the observer function to
NetworkPropertiesUpdated.

I also renamed NetworkStateChanged to NetworkConnectionStateChanged, missed that
in the last change. This method (and the other methods) should only get called
when something actually changes.

               managed->AsNetworkState(), iter.key(), iter.value())) {
         network_property_changed = true;
       }
     } else {
       managed->PropertyChanged(iter.key(), iter.value());
     }
   }
   // Notify observers.
   if (network_property_changed) {
     NetworkState* network = managed->AsNetworkState();
     DCHECK(network);
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                      NetworkServiceChanged(network));
+                      NetworkPropertyChanged(network));
   }
   network_event_log::AddEntry(
       kLogModule, "PropertiesReceived",
       StringPrintf("%s (%s)", path.c_str(), managed->name().c_str()));
 }
 
 void NetworkStateHandler::UpdateNetworkServiceProperty(
     const std::string& service_path,
     const std::string& key,
     const base::Value& value) {
   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));
-  }
+  if (!ParseNetworkServiceProperty(network, key, value))
+    return;
+  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(
     const std::string& service_path,
     const std::string& ip_address) {
   NetworkState* network = GetModifiableNetworkState(service_path);
   if (!network)
     return;
   std::string detail = network->name() + ".IPAddress = " + ip_address;
   network_event_log::AddEntry(kLogModule, "NetworkIPChanged", detail);
   network->set_ip_address(ip_address);
   FOR_EACH_OBSERVER(
       NetworkStateHandlerObserver, observers_,
-      NetworkServiceChanged(network));
+      NetworkPropertyChanged(network));
 }
 
 void NetworkStateHandler::ManagerPropertyChanged() {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkManagerChanged());
 }
 
 void NetworkStateHandler::ManagedStateListChanged(
     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     // Notify observers that the list of networks has changed.
     NetworkStateList network_list;
     GetNetworkList(&network_list);
     network_event_log::AddEntry(
         kLogModule, "NetworkListChanged",
         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));
-    }
+    // The list order may have changed, so check if the default network changed.
+    CheckDefaultNetworkChanged();
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     network_event_log::AddEntry(
         kLogModule, "DeviceListChanged",
         StringPrintf("Size: %"PRIuS, device_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       DeviceListChanged());
   } else {
     NOTREACHED();
   }
 }
@@ skipping 40 matching lines @@
   }
   NOTREACHED();
   return NULL;
 }
 
 bool NetworkStateHandler::ParseNetworkServiceProperty(
     NetworkState* network,
     const std::string& key,
     const base::Value& value) {
   DCHECK(network);
+  std::string prev_connection_state = network->connection_state();
   if (!network->PropertyChanged(key, value))
     return false;
-  if (network->path() == active_network_path_ &&
-      key == flimflam::kStateProperty) {
+  if (key == flimflam::kStateProperty &&
+      network->connection_state() != prev_connection_state)
+    OnNetworkConnectionStateChanged(network);

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

looks better. Now it's clearer which conditions are checked before a
notification about the state change is send out.

+  return true;
+}
+
+bool NetworkStateHandler::OnNetworkConnectionStateChanged(
+    NetworkState* network) {
+  std::string desc = StringPrintf(
+      "%s: %s", network->path().c_str(), network->connection_state().c_str());
+  network_event_log::AddEntry(
+      kLogModule, "NetworkStateChanged", desc);
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                    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_,
-                      ActiveNetworkStateChanged(network));
+                      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