Request updates when network state changes

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 e3f3dd36723214c4158be45e22ca620f2fc92233..6f3d960939040b727b5c417af23990af7667898d 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -440,7 +440,7 @@ void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
     }
     list_entries.insert(path);
   }
-  // Delete any remaning entries in managed_map.
+  // Delete any remaining entries in managed_map.
   STLDeleteContainerPairSecondPointers(managed_map.begin(), managed_map.end());
 }
 
@@ -524,29 +524,43 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   if (!network)
     return;
   std::string prev_connection_state = network->connection_state();
+  std::string prev_profile_path = network->profile_path();
   if (!network->PropertyChanged(key, value))
     return;
 
   if (key == flimflam::kStateProperty) {
-    if (ConnectionStateChanged(network, prev_connection_state))
+    if (ConnectionStateChanged(network, prev_connection_state)) {
       OnNetworkConnectionStateChanged(network);
+      // If the connection state changes, other properties such as IPConfig
+      // may have changed, so request a full update.
+      RequestUpdateForNetwork(service_path);
+    }
   } else {
-    if (network->path() == default_network_path_ &&
-        key != flimflam::kSignalStrengthProperty &&
-        key != shill::kWifiFrequencyListProperty) {
-      // WiFi SignalStrength and WifiFrequencyList updates are too noisy, so
+    bool noisy_property =
+        key == flimflam::kSignalStrengthProperty ||
+        key == shill::kWifiFrequencyListProperty;

[pneubeck (no reviews), 2013/08/08 07:40:56]

nit:
I don't say you should, but you could remove kWifiFrequencyListProperty, as it
isn't parsed by NetworkState anymore.

+    if (network->path() == default_network_path_ && !noisy_property) {
+      // Wifi SignalStrength and WifiFrequencyList updates are too noisy, so
       // don't trigger default network updates for those changes.
       OnDefaultNetworkChanged();
     }
-    std::string detail = network->name() + "." + key;
-    detail += " = " + network_event_log::ValueAsString(value);
-    network_event_log::LogLevel log_level = network_event_log::LOG_LEVEL_EVENT;
-    if (key == flimflam::kErrorProperty || key == shill::kErrorDetailsProperty)
-      log_level = network_event_log::LOG_LEVEL_ERROR;
-    else if (key == flimflam::kSignalStrengthProperty ||
-             key == shill::kWifiFrequencyListProperty)
-      log_level = network_event_log::LOG_LEVEL_DEBUG;
-    NET_LOG_LEVEL(log_level, "NetworkPropertyUpdated", detail);
+    if (prev_profile_path.empty() && !network->profile_path().empty()) {
+      // If added to a Profile, request a full update so that a FavoriteState
+      // gets created.
+      RequestUpdateForNetwork(service_path);
+    }
+    if (!noisy_property) {
+      std::string detail = network->name() + "." + key;
+      detail += " = " + network_event_log::ValueAsString(value);
+      network_event_log::LogLevel log_level;
+      if (key == flimflam::kErrorProperty ||
+          key == shill::kErrorDetailsProperty) {
+        log_level = network_event_log::LOG_LEVEL_ERROR;
+      } else {
+        log_level = network_event_log::LOG_LEVEL_EVENT;

[pneubeck (no reviews), 2013/08/08 07:40:56]

nit: you could initialize log_level with LOG_LEVEL_EVENT and remove the else.

[stevenjb, 2013/08/08 16:42:30]

I had that first but for some reason it looked less clear to me.

+      }
+      NET_LOG_LEVEL(log_level, "NetworkPropertyUpdated", detail);
+    }
   }
   NetworkPropertiesUpdated(network);
 }
@@ -596,8 +610,8 @@ void NetworkStateHandler::ManagedStateListChanged(
   } else if (type == ManagedState::MANAGED_TYPE_FAVORITE) {
     NET_LOG_DEBUG("FavoriteListChanged",
                   base::StringPrintf("Size:%" PRIuS, favorite_list_.size()));
-    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
-                      NetworkListChanged());
+    // This will always get triggered for TYPE_NETWORK if the favorite list

[pneubeck (no reviews), 2013/08/08 07:40:56]

I know the reason for this change, but I don't quite understand the comment.

The commit message is clearer, or maybe:
This will always get triggered if the NetworkList changed, so do not notify the
observers about NetworkListChanged again.

[stevenjb, 2013/08/08 16:42:30]

Clarified.

+    // changes, so do not call observers again here.
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     NET_LOG_DEBUG("DeviceListChanged",
                   base::StringPrintf("Size:%" PRIuS, device_list_.size()));
@@ -690,7 +704,7 @@ void NetworkStateHandler::NetworkPropertiesUpdated(
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkPropertiesUpdated(network));
   // If |connecting_network_| transitions to a non-idle, non-connecting state,
-  // clear it *after* signalling observers.
+  // clear it *after* signaling observers.
   if (network->path() == connecting_network_ &&
       !network->IsConnectingState() &&
       network->connection_state() != flimflam::kStateIdle) {