Use GUID instead of ServicePath in networkingPrivate API

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"
@@ skipping 283 matching lines @@
 
 void NetworkStateHandler::GetFavoriteListByType(const NetworkTypePattern& type,
                                                 FavoriteStateList* list) const {
   DCHECK(list);
   FavoriteStateList result;
   list->clear();
   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->IsFavorite() &&
+    if (favorite->update_received() && favorite->IsInProfile() &&
         favorite->Matches(type)) {
       list->push_back(favorite);
     }
   }
 }
 
 const FavoriteState* NetworkStateHandler::GetFavoriteState(
     const std::string& service_path) const {
   ManagedState* managed =
       GetModifiableManagedState(&favorite_list_, service_path);
   if (!managed)
     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();
 }
 
 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 140 matching lines @@
   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 = 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();
 
   std::string desc = GetManagedStateLogType(managed) + " Properties Received";
   NET_LOG_DEBUG(desc, GetManagedStateLogName(managed));
 
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     UpdateNetworkStateProperties(managed->AsNetworkState(), properties);
   } else {
     // Device, Favorite
     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);
@@ skipping 154 matching lines @@
     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->IsFavorite())
+      if (!favorite->IsInProfile())
         continue;
       if (favorite->IsPrivate())
         ++unshared;
       else
         ++shared;
     }
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedShared", shared);
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedUnshared", unshared);
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     NotifyDeviceListChanged();
@@ skipping 26 matching lines @@
     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) {
+    // 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()));
   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;
@@ skipping 97 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