Use GUID instead of ServicePath in networkingPrivate API

chromeos/network/network_state_handler.h

Index: chromeos/network/network_state_handler.h
diff --git a/chromeos/network/network_state_handler.h b/chromeos/network/network_state_handler.h
index 314a975ba1a9e8655116f3858fcfde08f06af116..c6d4c2db721504132d9a59f7dddd6c4daa17afe0 100644
--- a/chromeos/network/network_state_handler.h
+++ b/chromeos/network/network_state_handler.h
@@ -49,6 +49,21 @@ class NetworkTypePattern;
 // keep properties up to date by managing the appropriate Shill observers.
 // It will invoke its own more specific observer methods when the specified
 // changes occur.
+//
+// Some notes about NetworkState, FavoriteState, and GUIDs:
+// * A FavoriteState exists for all network services stored in a profile, and
+//   all "visible" networks (physically connected networks like ethernet and
+//   cellular or in-range wifi networks). If the network is stored in a profile,
+//   FavoriteState.IsInProfile() will return true.
+// * A NetworkState exists for "visible" networks only. There will always be a
+//   corresponding FavoriteState with the same service_path() property.
+// * All networks saved to a profile will have a saved GUID that is persistent
+//   across sessions.
+// * Networks that are not saved to a profile will have a GUID assigned when
+//   the initial properties are received. The GUID will be consistent for
+//   the duration of a session, even if the network drops out and returns.
+// * Both FavoriteState and NetworkState store the GUID. It will always be the
+//   same for the same network (i.e. entries with the same service_path()).
 
 class CHROMEOS_EXPORT NetworkStateHandler
     : public internal::ShillPropertyHandler::Listener {
@@ -175,6 +190,11 @@ class CHROMEOS_EXPORT NetworkStateHandler
   // Finds and returns a favorite state by |service_path| or NULL if not found.
   const FavoriteState* GetFavoriteState(const std::string& service_path) const;
 
+  // Finds the FavoriteState associated with |guid|. Note: The FavoriteState
+  // may or may not be in a profile (IsProfile() may be false), and may or may

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

it's a bit unlucky that the name is so similar to the above GetFavorite*
functions but the behavior is rather different (IsProfile() may be false).

to reduce mistakes, maybe add a comment at the other GetFavorite* functions that
it 'ensures that IsProfile() is true';
or rename this one to 'GetStateFromGuid'.

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

GetFavoriteState -> GetFravoriteStateFromServicePath with 'is_configured' as an
argument (which I think is a useful clarification to any code that calls it).

[pneubeck (no reviews), 2014/05/13 08:43:54]

yes, that's much better.

+  // not be visible.
+  const FavoriteState* GetFavoriteStateFromGuid(const std::string& guid) const;
+
   // Requests a network scan. This may trigger updates to the network
   // list, which will trigger the appropriate observer calls.
   void RequestScan() const;
@@ -288,6 +308,7 @@ class CHROMEOS_EXPORT NetworkStateHandler
  private:
   typedef std::list<base::Closure> ScanCallbackList;
   typedef std::map<std::string, ScanCallbackList> ScanCompleteCallbackMap;
+  typedef std::map<std::string, std::string> SpecifierGuidMap;
   friend class NetworkStateHandlerTest;
   FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest, NetworkStateHandlerStub);
 
@@ -296,6 +317,10 @@ class CHROMEOS_EXPORT NetworkStateHandler
   void UpdateNetworkStateProperties(NetworkState* network,
                                     const base::DictionaryValue& properties);
 
+  // Ensure a valid GUID for FavoriteState and update the NetworkState GUID from
+  // the corresponding FavoriteState if necessary.
+  void UpdateGuid(ManagedState* managed);
+
   // Sends DeviceListChanged() to observers and logs an event.
   void NotifyDeviceListChanged();
 
@@ -305,6 +330,8 @@ class CHROMEOS_EXPORT NetworkStateHandler
   DeviceState* GetModifiableDeviceState(const std::string& device_path) const;
   NetworkState* GetModifiableNetworkState(
       const std::string& service_path) const;
+  FavoriteState* GetModifiableFavoriteState(
+      const std::string& service_path) const;
   ManagedState* GetModifiableManagedState(const ManagedStateList* managed_list,
                                           const std::string& path) const;
 
@@ -357,6 +384,9 @@ class CHROMEOS_EXPORT NetworkStateHandler
   // Callbacks to run when a scan for the technology type completes.
   ScanCompleteCallbackMap scan_complete_callbacks_;
 
+  // Map of network specifiers to guids.

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

According to your class comment, you should ensure the invariant:
'Contains a key /exactly/ for each specifier of every visible network with
IsProfile()==false'
(i.e. no entry if IsProfile()==true)

please comment it here.

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

Done.

+  SpecifierGuidMap specifier_guid_map_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);
 };