Merge FavoriteState into NetworkState

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/guid.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chromeos/network/device_state.h"
-#include "chromeos/network/favorite_state.h"
 #include "chromeos/network/managed_state.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "chromeos/network/shill_property_handler.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 namespace {
 
 bool ConnectionStateChanged(NetworkState* network,
                             const std::string& prev_connection_state) {
   return (network->connection_state() != prev_connection_state) &&
          (network->connection_state() != shill::kStateIdle ||
           !prev_connection_state.empty());
 }
 
 std::string GetManagedStateLogType(const ManagedState* state) {
   switch (state->managed_type()) {
     case ManagedState::MANAGED_TYPE_NETWORK:
       return "Network";
-    case ManagedState::MANAGED_TYPE_FAVORITE:
-      return "Favorite";
     case ManagedState::MANAGED_TYPE_DEVICE:
       return "Device";
   }
   NOTREACHED();
   return "";
 }
 
-std::string GetManagedStateLogName(const ManagedState* state) {
+std::string GetLogName(const ManagedState* state) {
   if (!state)
     return "None";
   return base::StringPrintf("%s (%s)", state->name().c_str(),
                             state->path().c_str());
 }
 
 }  // namespace
 
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
     "ethernet,wifi,cellular";
 
 NetworkStateHandler::NetworkStateHandler() {
 }
 
 NetworkStateHandler::~NetworkStateHandler() {
   STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
-  STLDeleteContainerPointers(favorite_list_.begin(), favorite_list_.end());
   STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
 void NetworkStateHandler::InitShillPropertyHandler() {
   shill_property_handler_.reset(new internal::ShillPropertyHandler(this));
   shill_property_handler_->Init();
 }
 
 // static
 NetworkStateHandler* NetworkStateHandler::InitializeForTest() {
@@ skipping 105 matching lines @@
     return NULL;
   return network;
 }
 
 const NetworkState* NetworkStateHandler::DefaultNetwork() const {
   if (default_network_path_.empty())
     return NULL;
   return GetNetworkState(default_network_path_);
 }
 
-const FavoriteState* NetworkStateHandler::DefaultFavoriteNetwork() const {
-  const NetworkState* default_network = DefaultNetwork();
-  if (!default_network)
-    return NULL;
-  const FavoriteState* default_favorite = GetFavoriteStateFromServicePath(
-      default_network->path(), true /* configured_only */);
-  DCHECK(default_network->type() != shill::kTypeWifi || default_favorite)
-      << "No favorite for: " << default_network->path();
-  DCHECK(!default_favorite || default_favorite->update_received())
-      << "No update received for: " << default_network->path();
-  return default_favorite;
-}
-
 const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
     const NetworkTypePattern& type) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->update_received())
       continue;
-    if (!network->IsConnectedState())
+    if (!network->visible() || !network->IsConnectedState())

[pneubeck (no reviews), 2014/06/10 10:51:37]

Hm. If it's not visible but in a connected state, isn't that a bug?
Better dcheck/log an error?

[stevenjb, 2014/06/10 16:17:28]

It's not necessarily a bug, we don't clear state when a network is removed from
the visible list. We shouldn't ever be checking State on non visible networks.
The tests aren't related to each other, I could separate them if that would be
more clear.

[pneubeck (no reviews), 2014/06/11 12:44:55]

Ok. Got it. But considering the documentation of IsConnectedState (it doesn't
mention the visibility) and that this behavior is likely to be surprising to new
readers, I would suggest just changing the the connection_state() accessor to
return Idle if !visible() . (I wouldn't update the state in the set_visible()
function; as that would then rely on the order in which visible and connection
state are updated).

[stevenjb, 2014/06/11 23:31:40]

I went ahead and changed connection_state() to to return kDisconnect if not
visible.

       break;  // Connected networks are listed first.
     if (network->Matches(type))
       return network;
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
     const NetworkTypePattern& type) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->update_received() || network->IsConnectedState())
       continue;
-    if (!network->IsConnectingState())
+    if (!network->visible() || !network->IsConnectingState())

[pneubeck (no reviews), 2014/06/10 10:51:37]

ditto

[stevenjb, 2014/06/10 16:17:28]

ditto

       break;  // Connected and connecting networks are listed first.
     if (network->Matches(type))
       return network;
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::FirstNetworkByType(
     const NetworkTypePattern& type) const {
   for (ManagedStateList::const_iterator iter = network_list_.begin();
@@ skipping 14 matching lines @@
   const NetworkState* network = ConnectedNetworkByType(type);
   if (network)
     device = GetDeviceState(network->device_path());
   else
     device = GetDeviceStateByType(type);
   if (!device)
     return std::string();
   return network_util::FormattedMacAddress(device->mac_address());
 }
 
-void NetworkStateHandler::GetNetworkList(NetworkStateList* list) const {
-  GetNetworkListByType(NetworkTypePattern::Default(), list);
+void NetworkStateHandler::GetVisibleNetworkListByType(
+    const NetworkTypePattern& type,
+    NetworkStateList* list) const {
+  GetNetworkListByType(type,
+                       false /* configured_only */,
+                       true /* visible_only */,
+                       0 /* no limit */,
+                       list);
+}
+
+void NetworkStateHandler::GetVisibleNetworkList(NetworkStateList* list) const {
+  GetVisibleNetworkListByType(NetworkTypePattern::Default(), list);
 }
 
 void NetworkStateHandler::GetNetworkListByType(const NetworkTypePattern& type,
+                                               bool configured_only,
+                                               bool visible_only,
+                                               int limit,
                                                NetworkStateList* list) const {
   DCHECK(list);
   list->clear();
+  NetworkStateList result;

[pneubeck (no reviews), 2014/06/10 10:51:37]

unused

[stevenjb, 2014/06/10 16:17:28]

Done.

+  int count = 0;
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
-    if (network->update_received() && network->Matches(type))
-      list->push_back(network);
+    if (!network->update_received() || !network->Matches(type))
+      continue;
+    if (configured_only && !network->IsInProfile())
+      continue;
+    if (visible_only && !network->visible())
+      continue;
+    list->push_back(network);
+    if (limit > 0 && ++count >= limit)
+      break;
   }
 }
 
+const NetworkState* NetworkStateHandler::GetNetworkStateFromServicePath(
+    const std::string& service_path,
+    bool configured_only) const {
+  ManagedState* managed =
+      GetModifiableManagedState(&network_list_, service_path);
+  if (!managed)
+    return NULL;
+  const NetworkState* network = managed->AsNetworkState();
+  DCHECK(network);
+  if (!network->update_received() ||
+      (configured_only && !network->IsInProfile())) {
+    return NULL;
+  }
+  return network;
+}
+
+const NetworkState* NetworkStateHandler::GetNetworkStateFromGuid(
+    const std::string& guid) const {
+  DCHECK(!guid.empty());
+  for (ManagedStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = (*iter)->AsNetworkState();
+    if (network->guid() == guid)
+      return network;
+  }
+  return NULL;
+}
+
 void NetworkStateHandler::GetDeviceList(DeviceStateList* list) const {
   GetDeviceListByType(NetworkTypePattern::Default(), list);
 }
 
 void NetworkStateHandler::GetDeviceListByType(const NetworkTypePattern& type,
                                               DeviceStateList* list) const {
   DCHECK(list);
   list->clear();
   for (ManagedStateList::const_iterator iter = device_list_.begin();
        iter != device_list_.end(); ++iter) {
     const DeviceState* device = (*iter)->AsDeviceState();
     DCHECK(device);
     if (device->update_received() && device->Matches(type))
       list->push_back(device);
   }
 }
 
-void NetworkStateHandler::GetFavoriteList(FavoriteStateList* list) const {
-  GetFavoriteListByType(NetworkTypePattern::Default(),
-                        true /* configured_only */,
-                        false /* visible_only */,
-                        0 /* no limit */,
-                        list);
-}
-
-void NetworkStateHandler::GetFavoriteListByType(const NetworkTypePattern& type,
-                                                bool configured_only,
-                                                bool visible_only,
-                                                int limit,
-                                                FavoriteStateList* list) const {
-  DCHECK(list);
-  std::set<std::string> visible_networks;
-  if (visible_only) {
-    // Prepare a set of visible network service paths for fast lookup.
-    for (ManagedStateList::const_iterator iter = network_list_.begin();
-         iter != network_list_.end(); ++iter) {
-      visible_networks.insert((*iter)->path());
-    }
-  }
-  FavoriteStateList result;
-  list->clear();
-  int count = 0;
-  for (ManagedStateList::const_iterator iter = favorite_list_.begin();
-       iter != favorite_list_.end(); ++iter) {
-    const FavoriteState* favorite = (*iter)->AsFavoriteState();
-    DCHECK(favorite);
-    if (!favorite->update_received() || !favorite->Matches(type))
-      continue;
-    if (configured_only && !favorite->IsInProfile())
-      continue;
-    if (visible_only && !ContainsKey(visible_networks, favorite->path()))
-      continue;
-    list->push_back(favorite);
-    if (limit > 0 && ++count >= limit)
-      break;
-  }
-}
-
-const FavoriteState* NetworkStateHandler::GetFavoriteStateFromServicePath(
-    const std::string& service_path,
-    bool configured_only) const {
-  ManagedState* managed =
-      GetModifiableManagedState(&favorite_list_, service_path);
-  if (!managed)
-    return NULL;
-  const FavoriteState* favorite = managed->AsFavoriteState();
-  DCHECK(favorite);
-  if (!favorite->update_received() ||
-      (configured_only && !favorite->IsInProfile())) {
-    return NULL;
-  }
-  return favorite;
-}
-
-const FavoriteState* NetworkStateHandler::GetFavoriteStateFromGuid(
-    const std::string& guid) const {
-  DCHECK(!guid.empty());
-  for (ManagedStateList::const_iterator iter = favorite_list_.begin();
-       iter != favorite_list_.end(); ++iter) {
-    const FavoriteState* favorite = (*iter)->AsFavoriteState();
-    if (favorite->guid() == guid)
-      return favorite;
-  }
-  return NULL;
-}
-
 void NetworkStateHandler::RequestScan() const {
   NET_LOG_USER("RequestScan", "");
   shill_property_handler_->RequestScan();
 }
 
 void NetworkStateHandler::WaitForScan(const std::string& type,
                                       const base::Closure& callback) {
   scan_complete_callbacks_[type].push_back(callback);
   if (!GetScanningByType(NetworkTypePattern::Primitive(type)))
     RequestScan();
@@ skipping 22 matching lines @@
   if (network)
     network->clear_last_error();
 }
 
 void NetworkStateHandler::SetCheckPortalList(
     const std::string& check_portal_list) {
   NET_LOG_EVENT("SetCheckPortalList", check_portal_list);
   shill_property_handler_->SetCheckPortalList(check_portal_list);
 }
 
-const FavoriteState* NetworkStateHandler::GetEAPForEthernet(
+const NetworkState* NetworkStateHandler::GetEAPForEthernet(
     const std::string& service_path) const {
   const NetworkState* network = GetNetworkState(service_path);
   if (!network) {
     NET_LOG_ERROR("GetEAPForEthernet", "Unknown service path " + service_path);
     return NULL;
   }
   if (network->type() != shill::kTypeEthernet) {
     NET_LOG_ERROR("GetEAPForEthernet", "Not of type Ethernet: " + service_path);
     return NULL;
   }
   if (!network->IsConnectedState())
     return NULL;
 
   // The same EAP service is shared for all ethernet services/devices.
   // However EAP is used/enabled per device and only if the connection was
   // successfully established.
   const DeviceState* device = GetDeviceState(network->device_path());
   if (!device) {
     NET_LOG_ERROR(
         "GetEAPForEthernet",
         base::StringPrintf("Unknown device %s of connected ethernet service %s",
                            network->device_path().c_str(),
                            service_path.c_str()));
     return NULL;
   }
   if (!device->eap_authentication_completed())
     return NULL;
 
-  FavoriteStateList list;
-  GetFavoriteListByType(NetworkTypePattern::Primitive(shill::kTypeEthernetEap),
-                        true /* configured_only */,
-                        false /* visible_only */,
-                        1 /* limit */,
-                        &list);
+  NetworkStateList list;
+  GetNetworkListByType(NetworkTypePattern::Primitive(shill::kTypeEthernetEap),
+                       true /* configured_only */,
+                       false /* visible_only */,
+                       1 /* limit */,
+                       &list);
   if (list.empty()) {
     NET_LOG_ERROR("GetEAPForEthernet",
                   base::StringPrintf(
                       "Ethernet service %s connected using EAP, but no "
                       "EAP service found.",
                       service_path.c_str()));
     return NULL;
   }
   return list.front();
 }
 
 //------------------------------------------------------------------------------
 // ShillPropertyHandler::Delegate overrides
 
 void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
                                             const base::ListValue& entries) {
   ManagedStateList* managed_list = GetManagedList(type);
-  NET_LOG_DEBUG(base::StringPrintf("UpdateManagedList:%d", type),
+  NET_LOG_DEBUG("UpdateManagedList: " + ManagedState::TypeToString(type),
                 base::StringPrintf("%" PRIuS, entries.GetSize()));
   // Create a map of existing entries. Assumes all entries in |managed_list|
   // are unique.
-  std::map<std::string, ManagedState*> managed_map;
+  typedef std::map<std::string, ManagedState*> ManagedMap;
+  ManagedMap managed_map;
   for (ManagedStateList::iterator iter = managed_list->begin();
        iter != managed_list->end(); ++iter) {
     ManagedState* managed = *iter;
     DCHECK(!ContainsKey(managed_map, managed->path()));
     managed_map[managed->path()] = managed;
   }
   // Clear the list (pointers are temporarily owned by managed_map).
   managed_list->clear();
   // Updates managed_list and request updates for new entries.
   std::set<std::string> list_entries;
   for (base::ListValue::const_iterator iter = entries.begin();
        iter != entries.end(); ++iter) {
     std::string path;
     (*iter)->GetAsString(&path);
     if (path.empty() || path == shill::kFlimflamServicePath) {
       NET_LOG_ERROR(base::StringPrintf("Bad path in list:%d", type), path);
       continue;
     }
-    std::map<std::string, ManagedState*>::iterator found =
-        managed_map.find(path);
-    ManagedState* managed;
+    ManagedMap::iterator found = managed_map.find(path);
     if (found == managed_map.end()) {
       if (list_entries.count(path) != 0) {
         NET_LOG_ERROR("Duplicate entry in list", path);
         continue;
       }
-      managed = ManagedState::Create(type, path);
+      ManagedState* managed = ManagedState::Create(type, path);
       managed_list->push_back(managed);
     } else {
-      managed = found->second;
-      managed_list->push_back(managed);
+      managed_list->push_back(found->second);
       managed_map.erase(found);
     }
+    NET_LOG_DEBUG("Managed Entry: " + ManagedState::TypeToString(type), path);

[pneubeck (no reviews), 2014/06/10 10:51:37]

optional nit: too verbose even for debug? Your call, since you're the primary
user of the net log.

[stevenjb, 2014/06/10 16:17:28]

Was debugging. Removed.

     list_entries.insert(path);
   }
   // 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 = favorite_list_.begin();
-       iter != favorite_list_.end(); ++iter) {
-    shill_property_handler_->RequestProperties(
-        ManagedState::MANAGED_TYPE_NETWORK, (*iter)->path());
+void NetworkStateHandler::UpdateVisibleNetworks(

[pneubeck (no reviews), 2014/06/10 10:51:37]

if this is called because of a kServices change before the kServiceCompleteList
change is signaled, then we ignore networks that are not known (line 493) and
pick up visibility for these networks only on the next list change.

[stevenjb, 2014/06/10 16:17:27]

Correct. Shill emits these in the order we desire, but this is less than ideal
and will be fixed in a followup when we ignore Services and use
Service.Visibility instead.

+    const base::ListValue& entries) {
+  NET_LOG_DEBUG(base::StringPrintf("UpdateVisibleNetworks"),
+                base::StringPrintf("%" PRIuS, entries.GetSize()));
+  // Create a map of all networks and clear the visible state.
+  ManagedStateList* network_list =
+      GetManagedList(ManagedState::MANAGED_TYPE_NETWORK);
+  typedef std::map<std::string, NetworkState*> NetworkMap;
+  NetworkMap network_map;
+  for (ManagedStateList::iterator iter = network_list->begin();
+       iter != network_list->end(); ++iter) {
+    NetworkState* network = (*iter)->AsNetworkState();
+    network_map[network->path()] = network;
+    network->set_visible(false);
+  }
+  // Look up each entry and set the associated network to visible.
+  for (base::ListValue::const_iterator iter = entries.begin();
+       iter != entries.end(); ++iter) {
+    std::string path;
+    (*iter)->GetAsString(&path);
+    NetworkMap::iterator found = network_map.find(path);
+    if (found != network_map.end())
+      found->second->set_visible(true);
+    else
+      NET_LOG_DEBUG("Visible network not in list", path);
   }
 }
 
+void NetworkStateHandler::ProfileListChanged() {
+  NET_LOG_EVENT("ProfileListChanged", "Re-Requesting Network Properties");
+  for (ManagedStateList::iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    NetworkState* network = (*iter)->AsNetworkState();
+    DCHECK(network);
+    if (!network->IsInProfile())

[pneubeck (no reviews), 2014/06/10 10:51:37]

The old function updated all services from ServicesCompleteList, now you're
restricting it to configured/stored ones.

Seems ok to me, I just wanted to point out the difference.

[stevenjb, 2014/06/10 16:17:28]

Yes, that was intentional, but not that you mention it, since we get the profile
path from the service itself (instead of from the Profile) we should probably go
ahead and request all services to be safe. This only ever happens once per
session currently, and will never happen often.

+      continue;
+    shill_property_handler_->RequestProperties(
+        ManagedState::MANAGED_TYPE_NETWORK, network->path());
+  }
+}
+
 void NetworkStateHandler::UpdateManagedStateProperties(
     ManagedState::ManagedType type,
     const std::string& path,
     const base::DictionaryValue& properties) {
   ManagedStateList* managed_list = GetManagedList(type);
   ManagedState* managed = GetModifiableManagedState(managed_list, path);
   if (!managed) {
-    if (type != ManagedState::MANAGED_TYPE_FAVORITE) {
-      // The network has been removed from the list of visible networks.
-      NET_LOG_DEBUG("UpdateManagedStateProperties: Not found", path);
-      return;
-    }
-    // A Favorite may not have been created yet if it was added later (e.g.
-    // through ConfigureService) since ServiceCompleteList updates are not
-    // emitted. Add and update the state here.
-    managed = ManagedState::Create(type, path);
-    managed_list->push_back(managed);
+    // The network has been removed from the list of networks.
+    NET_LOG_DEBUG("UpdateManagedStateProperties: Not found", path);
+    return;
   }
   managed->set_update_received();
 
   std::string desc = GetManagedStateLogType(managed) + " Properties Received";
-  NET_LOG_DEBUG(desc, GetManagedStateLogName(managed));
+  NET_LOG_DEBUG(desc, GetLogName(managed));
 
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     UpdateNetworkStateProperties(managed->AsNetworkState(), properties);
   } else {
-    // Device, Favorite
+    // Device
     for (base::DictionaryValue::Iterator iter(properties);
          !iter.IsAtEnd(); iter.Advance()) {
       managed->PropertyChanged(iter.key(), iter.value());
     }
     managed->InitialPropertiesReceived(properties);
   }
-  UpdateGuid(managed);
   managed->set_update_requested(false);
 }
 
 void NetworkStateHandler::UpdateNetworkStateProperties(
     NetworkState* network,
     const base::DictionaryValue& properties) {
   DCHECK(network);
   bool network_property_updated = false;
   std::string prev_connection_state = network->connection_state();
   for (base::DictionaryValue::Iterator iter(properties);
        !iter.IsAtEnd(); iter.Advance()) {
     if (network->PropertyChanged(iter.key(), iter.value()))
       network_property_updated = true;
   }
   network_property_updated |= network->InitialPropertiesReceived(properties);
   // Notify observers of NetworkState changes.
   if (network_property_updated || network->update_requested()) {
     // Signal connection state changed after all properties have been updated.
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
-    NET_LOG_EVENT("NetworkPropertiesUpdated", GetManagedStateLogName(network));
+    NET_LOG_EVENT("NetworkPropertiesUpdated", GetLogName(network));
     NotifyNetworkPropertiesUpdated(network);
   }
+  UpdateGuid(network);
 }
 
 void NetworkStateHandler::UpdateNetworkServiceProperty(
     const std::string& service_path,
     const std::string& key,
     const base::Value& value) {
-  // Update any associated FavoriteState.
-  ManagedState* favorite =
-      GetModifiableManagedState(&favorite_list_, service_path);
   bool changed = false;
-  if (favorite)
-    changed |= 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();
   changed |= network->PropertyChanged(key, value);
   if (!changed)
     return;
 
   if (key == shill::kStateProperty) {
@@ skipping 28 matching lines @@
       } else {
         log_level = network_event_log::LOG_LEVEL_EVENT;
       }
       NET_LOG_LEVEL(log_level, log_event, detail);
     }
   }
 
   // All property updates signal 'NetworkPropertiesUpdated'.
   NotifyNetworkPropertiesUpdated(network);
 
-  // If added to a Profile, request a full update so that a FavoriteState
+  // If added to a Profile, request a full update so that a NetworkState
   // gets created.
   if (prev_profile_path.empty() && !network->profile_path().empty())
     RequestUpdateForNetwork(service_path);
 }
 
 void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
                                                const std::string& key,
                                                const base::Value& value) {
   DeviceState* device = GetModifiableDeviceState(device_path);
   if (!device)
     return;
   if (!device->PropertyChanged(key, value))
     return;
 
   std::string detail = device->name() + "." + key;
   detail += " = " + network_event_log::ValueAsString(value);
   NET_LOG_EVENT("DevicePropertyUpdated", detail);
 
   NotifyDeviceListChanged();
 
   if (key == shill::kScanningProperty && device->scanning() == false)
     ScanCompleted(device->type());
   if (key == shill::kEapAuthenticationCompletedProperty) {
     // Notify a change for each Ethernet service using this device.
     NetworkStateList ethernet_services;
-    GetNetworkListByType(NetworkTypePattern::Ethernet(), &ethernet_services);
+    GetNetworkListByType(NetworkTypePattern::Ethernet(),
+                         false /* configured_only */,
+                         false /* visible_only */,
+                         1 /* limit */,

[pneubeck (no reviews), 2014/06/10 10:51:37]

the comment and the used limit are off.
just don't use a limit to be on the safe side.

[stevenjb, 2014/06/10 16:17:28]

Done.

+                         &ethernet_services);
     for (NetworkStateList::const_iterator it = ethernet_services.begin();
          it != ethernet_services.end(); ++it) {
       const NetworkState* ethernet_service = *it;
       if (ethernet_service->update_received() ||
           ethernet_service->device_path() != device->path()) {
         continue;
       }
       RequestUpdateForNetwork(ethernet_service->path());
     }
   }
@@ skipping 21 matching lines @@
     const std::string& check_portal_list) {
   check_portal_list_ = check_portal_list;
 }
 
 void NetworkStateHandler::TechnologyListChanged() {
   // Eventually we would like to replace Technology state with Device state.
   // For now, treat technology state changes as device list changes.
   NotifyDeviceListChanged();
 }
 
 void NetworkStateHandler::ManagedStateListChanged(

[pneubeck (no reviews), 2014/06/10 10:51:36]

again, this is called on ServicesCompleteList changes but 'visible' counter
depends on Services being already updated.

[stevenjb, 2014/06/10 16:17:28]

It's called when all updates have been received so should be fine.

[pneubeck (no reviews), 2014/06/11 12:44:55]

_If_ any updates were requested. If no updates were requested, because only the
visibility changed, the behavior might be different.

[stevenjb, 2014/06/11 23:31:40]

Ugh, you're right, there is an annoying case there. I'll add an extra
ManagedStateListChanged() call when kServicesProperty is emitted. 

     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     // Notify observers that the list of networks has changed.
-    NET_LOG_EVENT("NetworkListChanged",
+    NET_LOG_EVENT("NOTIFY:NetworkListChanged",
                   base::StringPrintf("Size:%" PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged());
     // Update UMA stats.
-    UMA_HISTOGRAM_COUNTS_100("Networks.Visible", network_list_.size());
-  } else if (type == ManagedState::MANAGED_TYPE_FAVORITE) {
-    NET_LOG_DEBUG("FavoriteListChanged",
-                  base::StringPrintf("Size:%" PRIuS, favorite_list_.size()));
-    // The FavoriteState list only changes when the NetworkState list changes,
-    // so no need to signal observers here again.
-
-    // Update UMA stats.
-    size_t shared = 0, unshared = 0;
-    for (ManagedStateList::iterator iter = favorite_list_.begin();
-         iter != favorite_list_.end(); ++iter) {
-      FavoriteState* favorite = (*iter)->AsFavoriteState();
-      if (!favorite->IsInProfile())
-        continue;
-      if (favorite->IsPrivate())
-        ++unshared;
-      else
-        ++shared;
+    size_t shared = 0, unshared = 0, visible = 0;
+    for (ManagedStateList::iterator iter = network_list_.begin();
+         iter != network_list_.end(); ++iter) {
+      NetworkState* network = (*iter)->AsNetworkState();
+      if (network->visible())
+        ++visible;
+      if (network->IsInProfile()) {
+        if (network->IsPrivate())
+          ++unshared;
+        else
+          ++shared;
+      }
     }
+    UMA_HISTOGRAM_COUNTS_100("Networks.Visible", visible);
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedShared", shared);
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedUnshared", unshared);
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     std::string devices;
     for (ManagedStateList::const_iterator iter = device_list_.begin();
          iter != device_list_.end(); ++iter) {
       if (iter != device_list_.begin())
         devices += ", ";
       devices += (*iter)->name();
     }
-    NET_LOG_EVENT("DeviceList:", devices);
+    NET_LOG_EVENT("DeviceList", devices);
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
   }
 }
 
 void NetworkStateHandler::DefaultNetworkServiceChanged(
     const std::string& service_path) {
   // Shill uses '/' for empty service path values; check explicitly for that.
   const char* kEmptyServicePath = "/";
-  if (service_path == kEmptyServicePath)
-    default_network_path_.clear();
-  else
-    default_network_path_ = service_path;
+  std::string new_service_path =
+      (service_path != kEmptyServicePath) ? service_path : "";
+  if (new_service_path == default_network_path_)

[pneubeck (no reviews), 2014/06/10 10:51:36]

Just noting that this is an unrelated functional change.

[stevenjb, 2014/06/10 16:17:28]

Not entirely unrelated. This CL changes some of the timing and a lot of stub
behavior so we ended up calling this a bit more frequently. This change makes
the frequency more predictable.

[pneubeck (no reviews), 2014/06/11 12:44:55]

If you would want to separate it, you could commit this change first. I'm fine
with either.

+    return;
+
+  default_network_path_ = service_path;
   NET_LOG_EVENT("DefaultNetworkServiceChanged:", default_network_path_);
   const NetworkState* network = NULL;
   if (!default_network_path_.empty()) {
     network = GetNetworkState(default_network_path_);
     if (!network) {
       // If NetworkState is not available yet, do not notify observers here,
       // they will be notified when the state is received.
       NET_LOG_DEBUG("Default NetworkState not available",
                     default_network_path_);
       return;
     }
   }
   if (network && !network->IsConnectedState()) {
     NET_LOG_ERROR(
         "DefaultNetwork is not connected: " + network->connection_state(),
         network->path());
   }
   NotifyDefaultNetworkChanged(network);
 }
 
 //------------------------------------------------------------------------------
 // Private methods
 
-void NetworkStateHandler::UpdateGuid(ManagedState* managed) {
-  if (managed->managed_type() == ManagedState::MANAGED_TYPE_FAVORITE) {
-    FavoriteState* favorite = managed->AsFavoriteState();
-    std::string specifier = favorite->GetSpecifier();
-    if (!favorite->guid().empty()) {
-      // If the favorite is saved in a profile, remove the entry from the map.
-      // Otherwise ensure that the entry matches the specified GUID.
-      if (favorite->IsInProfile())
-        specifier_guid_map_.erase(specifier);
-      else
-        specifier_guid_map_[specifier] = favorite->guid();
-      return;
-    }
-    // Ensure that the FavoriteState has a valid GUID.
-    std::string guid;
-    SpecifierGuidMap::iterator iter = specifier_guid_map_.find(specifier);
-    if (iter != specifier_guid_map_.end()) {
-      guid = iter->second;
-    } else {
-      guid = base::GenerateGUID();
-      specifier_guid_map_[specifier] = guid;
-    }
-    favorite->SetGuid(guid);
-    NetworkState* network = GetModifiableNetworkState(favorite->path());
-    if (network)
-      network->SetGuid(guid);
-  } else if (managed->managed_type() == ManagedState::MANAGED_TYPE_NETWORK) {
-    // If the GUID is not set and a corresponding FavoriteState exists, get the
-    // GUID from the FavoriteState. Otherwise it will get set when the Favorite
-    // is created.
-    NetworkState* network = managed->AsNetworkState();
-    if (!network->guid().empty())
-      return;
-    // ShillPropertyHandler will always call UpdateManagedStateProperties with
-    // type FAVORITE before type NETWORK, so there should always be a
-    // corresponding FavoriteState here.
-    FavoriteState* favorite = GetModifiableFavoriteState(network->path());
-    DCHECK(favorite);
-    if (favorite && !favorite->guid().empty())
-      network->SetGuid(favorite->guid());
+void NetworkStateHandler::UpdateGuid(NetworkState* network) {
+  std::string specifier = network->GetSpecifier();

[pneubeck (no reviews), 2014/06/10 10:51:37]

optional nit: could add a DCHECK(!specifier.empty());

[stevenjb, 2014/06/10 16:17:28]

Done.

+  if (!network->guid().empty()) {
+    // If the network is saved in a profile, remove the entry from the map.
+    // Otherwise ensure that the entry matches the specified GUID. (e.g. in
+    // case a visible network with a specified guid gets configured with a
+    // new guid).
+    if (network->IsInProfile())
+      specifier_guid_map_.erase(specifier);
+    else
+      specifier_guid_map_[specifier] = network->guid();
+    return;
   }
+  // Ensure that the NetworkState has a valid GUID.
+  std::string guid;
+  SpecifierGuidMap::iterator iter = specifier_guid_map_.find(specifier);
+  if (iter != specifier_guid_map_.end()) {
+    guid = iter->second;
+  } else {
+    guid = base::GenerateGUID();
+    specifier_guid_map_[specifier] = guid;
+  }
+  network->SetGuid(guid);
 }
 
 void NetworkStateHandler::NotifyDeviceListChanged() {
-  NET_LOG_DEBUG("NotifyDeviceListChanged",
+  NET_LOG_DEBUG("NOTIFY:DeviceListChanged",
                 base::StringPrintf("Size:%" PRIuS, device_list_.size()));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DeviceListChanged());
 }
 
 DeviceState* NetworkStateHandler::GetModifiableDeviceState(
     const std::string& device_path) const {
   ManagedState* managed = GetModifiableManagedState(&device_list_, device_path);
   if (!managed)
     return NULL;
   return managed->AsDeviceState();
 }
 
 NetworkState* NetworkStateHandler::GetModifiableNetworkState(
     const std::string& service_path) const {
   ManagedState* managed =
       GetModifiableManagedState(&network_list_, service_path);
   if (!managed)
     return NULL;
   return managed->AsNetworkState();
 }
 
-FavoriteState* NetworkStateHandler::GetModifiableFavoriteState(
-    const std::string& service_path) const {
-  ManagedState* managed =
-      GetModifiableManagedState(&favorite_list_, service_path);
-  if (!managed)
-    return NULL;
-  return managed->AsFavoriteState();
-}
-
 ManagedState* NetworkStateHandler::GetModifiableManagedState(
     const ManagedStateList* managed_list,
     const std::string& path) const {
   for (ManagedStateList::const_iterator iter = managed_list->begin();
        iter != managed_list->end(); ++iter) {
     ManagedState* managed = *iter;
     if (managed->path() == path)
       return managed;
   }
   return NULL;
 }
 
 NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
     ManagedState::ManagedType type) {
   switch (type) {
     case ManagedState::MANAGED_TYPE_NETWORK:
       return &network_list_;
-    case ManagedState::MANAGED_TYPE_FAVORITE:
-      return &favorite_list_;
     case ManagedState::MANAGED_TYPE_DEVICE:
       return &device_list_;
   }
   NOTREACHED();
   return NULL;
 }
 
 void NetworkStateHandler::OnNetworkConnectionStateChanged(
     NetworkState* network) {
   DCHECK(network);
   std::string event = "NetworkConnectionStateChanged";
   if (network->path() == default_network_path_) {
     event = "Default" + event;
     if (!network->IsConnectedState()) {
       NET_LOG_ERROR(
           "DefaultNetwork is not connected: " + network->connection_state(),
           network->path());
     }
   }
-  NET_LOG_EVENT(event + ": " + network->connection_state(),
-                GetManagedStateLogName(network));
+  NET_LOG_EVENT("NOTIFY:" + event + ": " + network->connection_state(),
+                GetLogName(network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkConnectionStateChanged(network));
   if (network->path() == default_network_path_)
     NotifyDefaultNetworkChanged(network);
 }
 
 void NetworkStateHandler::NotifyDefaultNetworkChanged(
     const NetworkState* default_network) {
+  NET_LOG_EVENT("NOTIFY:DefaultNetworkChanged", GetLogName(default_network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DefaultNetworkChanged(default_network));
 }
 
 void NetworkStateHandler::NotifyNetworkPropertiesUpdated(
     const NetworkState* network) {
+  NET_LOG_DEBUG("NOTIFY:NetworkPropertiesUpdated", GetLogName(network));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkPropertiesUpdated(network));
 }
 
 void NetworkStateHandler::ScanCompleted(const std::string& type) {
   size_t num_callbacks = scan_complete_callbacks_.count(type);
   NET_LOG_EVENT("ScanCompleted",
                 base::StringPrintf("%s:%" PRIuS, type.c_str(), num_callbacks));
   if (num_callbacks == 0)
     return;
@@ skipping 43 matching lines @@
   if (type.MatchesType(shill::kTypeBluetooth))
     technologies.push_back(new std::string(shill::kTypeBluetooth));
   if (type.MatchesType(shill::kTypeVPN))
     technologies.push_back(new std::string(shill::kTypeVPN));
 
   CHECK_GT(technologies.size(), 0ul);
   return technologies.Pass();
 }
 
 }  // namespace chromeos