Request updates when network state changes

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/bind.h"
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/stl_util.h"
@@ skipping 422 matching lines @@
       }
       managed = ManagedState::Create(type, path);
       managed_list->push_back(managed);
     } else {
       managed = found->second;
       managed_list->push_back(managed);
       managed_map.erase(found);
     }
     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());
 }
 
 void NetworkStateHandler::ProfileListChanged() {
   NET_LOG_EVENT("ProfileListChanged", "Re-Requesting Network Properties");
   for (ManagedStateList::iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     shill_property_handler_->RequestProperties(
         ManagedState::MANAGED_TYPE_NETWORK, (*iter)->path());
   }
@@ skipping 63 matching lines @@
   ManagedState* favorite =
       GetModifiableManagedState(&favorite_list_, service_path);
   if (favorite)
     favorite->PropertyChanged(key, value);
 
   // Update the NetworkState.
   NetworkState* network = GetModifiableNetworkState(service_path);
   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);
 }
 
 void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
                                                const std::string& key,
                                                const base::Value& value) {
   DeviceState* device = GetModifiableDeviceState(device_path);
   if (!device)
     return;
@@ skipping 29 matching lines @@
     NET_LOG_EVENT("NetworkListChanged",
                   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();
   } 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()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       DeviceListChanged());
   } else {
     NOTREACHED();
   }
 }
 
@@ skipping 72 matching lines @@
                 GetManagedStateLogName(default_network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DefaultNetworkChanged(default_network));
 }
 
 void NetworkStateHandler::NetworkPropertiesUpdated(
     const NetworkState* network) {
   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) {
     connecting_network_.clear();
     NET_LOG_EVENT("ClearConnectingNetwork", base::StringPrintf(
         "%s:%s", GetManagedStateLogName(network).c_str(),
         network->connection_state().c_str()));
   }
 }
 
@@ skipping 21 matching lines @@
   }
   if (type == kMatchTypeDefault || type == kMatchTypeNonVirtual ||
       type == kMatchTypeWireless) {
     NOTREACHED();
     return flimflam::kTypeWifi;
   }
   return type;
 }
 
 }  // namespace chromeos