Merge FavoriteState into NetworkState

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 77b0b2744b6831fd005b90b656633289a1b03df5..f70d9062d58e4ba8e6dcd43d9555f2f38fef6da3 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -15,7 +15,6 @@
 #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"
@@ -38,8 +37,6 @@ 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";
   }
@@ -47,7 +44,7 @@ std::string GetManagedStateLogType(const ManagedState* state) {
   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(),
@@ -64,7 +61,6 @@ 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());
 }
 
@@ -190,19 +186,6 @@ const NetworkState* NetworkStateHandler::DefaultNetwork() const {
   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();
@@ -211,7 +194,7 @@ const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
     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;
@@ -227,7 +210,7 @@ const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
     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;
@@ -262,109 +245,90 @@ std::string NetworkStateHandler::FormattedHardwareAddressForType(
   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);
-  }
-}
-
-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))
+    if (!network->update_received() || !network->Matches(type))
       continue;
-    if (configured_only && !favorite->IsInProfile())
+    if (configured_only && !network->IsInProfile())
       continue;
-    if (visible_only && !ContainsKey(visible_networks, favorite->path()))
+    if (visible_only && !network->visible())
       continue;
-    list->push_back(favorite);
+    list->push_back(network);
     if (limit > 0 && ++count >= limit)
       break;
   }
 }
 
-const FavoriteState* NetworkStateHandler::GetFavoriteStateFromServicePath(
+const NetworkState* NetworkStateHandler::GetNetworkStateFromServicePath(
     const std::string& service_path,
     bool configured_only) const {
   ManagedState* managed =
-      GetModifiableManagedState(&favorite_list_, service_path);
+      GetModifiableManagedState(&network_list_, service_path);
   if (!managed)
     return NULL;
-  const FavoriteState* favorite = managed->AsFavoriteState();
-  DCHECK(favorite);
-  if (!favorite->update_received() ||
-      (configured_only && !favorite->IsInProfile())) {
+  const NetworkState* network = managed->AsNetworkState();
+  DCHECK(network);
+  if (!network->update_received() ||
+      (configured_only && !network->IsInProfile())) {
     return NULL;
   }
-  return favorite;
+  return network;
 }
 
-const FavoriteState* NetworkStateHandler::GetFavoriteStateFromGuid(
+const NetworkState* NetworkStateHandler::GetNetworkStateFromGuid(
     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;
+  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::RequestScan() const {
   NET_LOG_USER("RequestScan", "");
   shill_property_handler_->RequestScan();
@@ -407,7 +371,7 @@ void NetworkStateHandler::SetCheckPortalList(
   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) {
@@ -436,12 +400,12 @@ const FavoriteState* NetworkStateHandler::GetEAPForEthernet(
   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(
@@ -459,11 +423,12 @@ const FavoriteState* NetworkStateHandler::GetEAPForEthernet(
 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;
@@ -482,33 +447,63 @@ void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
       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::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 = favorite_list_.begin();
-       iter != favorite_list_.end(); ++iter) {
+  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, (*iter)->path());
+        ManagedState::MANAGED_TYPE_NETWORK, network->path());
   }
 }
 
@@ -519,33 +514,25 @@ void NetworkStateHandler::UpdateManagedStateProperties(
   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);
 }
 
@@ -566,23 +553,17 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
     // 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;
@@ -631,7 +612,7 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   // 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);
@@ -657,7 +638,11 @@ void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
   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;
@@ -703,30 +688,25 @@ void NetworkStateHandler::ManagedStateListChanged(
     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) {
@@ -737,7 +717,7 @@ void NetworkStateHandler::ManagedStateListChanged(
         devices += ", ";
       devices += (*iter)->name();
     }
-    NET_LOG_EVENT("DeviceList:", devices);
+    NET_LOG_EVENT("DeviceList", devices);
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
@@ -748,10 +728,12 @@ 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()) {
@@ -775,51 +757,33 @@ void NetworkStateHandler::DefaultNetworkServiceChanged(
 //------------------------------------------------------------------------------
 // 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());
@@ -842,15 +806,6 @@ NetworkState* NetworkStateHandler::GetModifiableNetworkState(
   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 {
@@ -868,8 +823,6 @@ NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
   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_;
   }
@@ -889,8 +842,8 @@ void NetworkStateHandler::OnNetworkConnectionStateChanged(
           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_)
@@ -899,12 +852,14 @@ void NetworkStateHandler::OnNetworkConnectionStateChanged(
 
 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));
 }