Use GUID instead of ServicePath in networkingPrivate API

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 1f061974199c01bbe503089b8bc4d85385524d23..125cefd93d29c47ffb580d08ef54e67d11e31c4e 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -6,6 +6,7 @@
 
 #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"
@@ -309,7 +310,7 @@ void NetworkStateHandler::GetFavoriteListByType(const NetworkTypePattern& type,
        iter != favorite_list_.end(); ++iter) {
     const FavoriteState* favorite = (*iter)->AsFavoriteState();
     DCHECK(favorite);
-    if (favorite->update_received() && favorite->IsFavorite() &&
+    if (favorite->update_received() && favorite->IsInProfile() &&
         favorite->Matches(type)) {
       list->push_back(favorite);
     }
@@ -324,11 +325,22 @@ const FavoriteState* NetworkStateHandler::GetFavoriteState(
     return NULL;
   const FavoriteState* favorite = managed->AsFavoriteState();
   DCHECK(favorite);
-  if (!favorite->update_received() || !favorite->IsFavorite())
+  if (!favorite->update_received() || !favorite->IsInProfile())
     return NULL;
   return favorite;
 }
 
+const FavoriteState* NetworkStateHandler::GetFavoriteStateFromGuid(
+    const std::string& guid) const {
+  for (ManagedStateList::const_iterator iter = favorite_list_.begin();
+       iter != favorite_list_.end(); ++iter) {
+    const FavoriteState* favorite = (*iter)->AsFavoriteState();

[pneubeck (no reviews), 2014/05/12 13:37:07]

skip if update_received()==false (might not be necessary if guid() return '' if
the required properties are not received)

[stevenjb, 2014/05/13 01:19:00]

guid() will be "" until update_received() is true. I'll DCHECK that 'guid' is
not empty.

+    if (favorite->guid() == guid)
+      return favorite;
+  }
+  return NULL;
+}
+
 void NetworkStateHandler::RequestScan() const {
   NET_LOG_USER("RequestScan", "");
   shill_property_handler_->RequestScan();
@@ -489,7 +501,7 @@ void NetworkStateHandler::UpdateManagedStateProperties(
     // 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 = new FavoriteState(path);
+    managed = ManagedState::Create(type, path);

[pneubeck (no reviews), 2014/05/12 13:37:07]

optional:
since |type| is constant, IMO the more specific 'new FavoriteState' is
preferable.

up to you.

[stevenjb, 2014/05/13 01:19:00]

I didn't like that NSH sometimes uses one and not the other. I was going to add
a Create wrapper and do some logic in there but I reverted that, but decided I
preferred keeping this. (I considered protecting the constructors but enough
tests and other code construct local *State members that I decided not to).

     managed_list->push_back(managed);
   }
   managed->set_update_received();
@@ -507,6 +519,7 @@ void NetworkStateHandler::UpdateManagedStateProperties(
     }
     managed->InitialPropertiesReceived(properties);
   }
+  UpdateGuid(managed);
   managed->set_update_requested(false);
 }
 
@@ -681,7 +694,7 @@ void NetworkStateHandler::ManagedStateListChanged(
     for (ManagedStateList::iterator iter = favorite_list_.begin();
          iter != favorite_list_.end(); ++iter) {
       FavoriteState* favorite = (*iter)->AsFavoriteState();
-      if (!favorite->IsFavorite())
+      if (!favorite->IsInProfile())
         continue;
       if (favorite->IsPrivate())
         ++unshared;
@@ -728,6 +741,38 @@ void NetworkStateHandler::DefaultNetworkServiceChanged(
 //------------------------------------------------------------------------------
 // Private methods
 
+void NetworkStateHandler::UpdateGuid(ManagedState* managed) {
+  if (managed->managed_type() == ManagedState::MANAGED_TYPE_FAVORITE) {
+    // Ensure that a FavoriteState has a valid GUID.

[pneubeck (no reviews), 2014/05/12 13:37:07]

nit: a -> the
(or even remove the comment)

[stevenjb, 2014/05/13 01:19:00]

Done.

+    FavoriteState* favorite = managed->AsFavoriteState();
+    if (!favorite->guid().empty())
+      return;
+    std::string specifier = favorite->GetSpecifier();
+    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

[pneubeck (no reviews), 2014/05/12 13:37:07]

hm.
There's a again a timing subtlety here, I think.
Usually, a NetworkState is 'valid' once the first full set of properties was
received and this was indicated by 'update_received()==true'.

With this change, could we have 'update_received()==true' and the guid still be
unset?
_Maybe_ this can't happen because you create the FavoriteState in
UpdateManagedStateProperties(
          ManagedState::MANAGED_TYPE_FAVORITE) before the NetworkState, but
that's at least not that obvious.

Could you make this independent of the order FavoriteState compared to
NetworkState? That would reduce the implicit dependencies on other code.

[stevenjb, 2014/05/13 01:19:00]

I don't think we can ever entirely guarantee that a FavoriteState exists for
every NetworkState, and I'm not sure that we should try.

I think that the correct thing to do is to remove guid() from NetworkState.
There are some places where we really should use FavoriteState instead of
NetworkState anyway. I looked into removing it and it's going to be a PITA, so I
will do it separately.

Meanwhile, if a network was saved, NetworkState will still have a guid as
before. If it wasn't saved, it will eventually have a guid, but might not, so we
can't write code that relies on it. I will add that to the comments.

+    // GUID from the FavoriteState. Otherwise it will get set when the Favorite
+    // is created.
+    NetworkState* network = managed->AsNetworkState();
+    if (!network->guid().empty())
+      return;
+    FavoriteState* favorite = GetModifiableFavoriteState(network->path());
+    if (favorite && !favorite->guid().empty())
+      network->SetGuid(favorite->guid());
+  }
+}
+
 void NetworkStateHandler::NotifyDeviceListChanged() {
   NET_LOG_DEBUG("NotifyDeviceListChanged",
                 base::StringPrintf("Size:%" PRIuS, device_list_.size()));
@@ -752,6 +797,15 @@ 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 {