Use GUID instead of ServicePath in networkingPrivate API

chromeos/network/network_state_handler.h

 // 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.
 
 #ifndef CHROMEOS_NETWORK_NETWORK_STATE_HANDLER_H_
 #define CHROMEOS_NETWORK_NETWORK_STATE_HANDLER_H_
 
 #include <map>
 #include <set>
 #include <string>
@@ skipping 31 matching lines @@
 // Class for tracking the list of visible networks and their properties.
 //
 // This class maps essential properties from the connection manager (Shill) for
 // each visible network. It is not used to change the properties of services or
 // devices, only global (manager) properties.
 //
 // All getters return the currently cached properties. This class is expected to
 // 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 {
  public:
   typedef std::vector<ManagedState*> ManagedStateList;
   typedef std::vector<const NetworkState*> NetworkStateList;
   typedef std::vector<const DeviceState*> DeviceStateList;
   typedef std::vector<const FavoriteState*> FavoriteStateList;
 
   enum TechnologyState {
@@ skipping 106 matching lines @@
   // favorite is visible and retrieve the complete properties (and vice-versa).
   void GetFavoriteList(FavoriteStateList* list) const;
 
   // Like GetFavoriteList() but only returns favorites with matching |type|.
   void GetFavoriteListByType(const NetworkTypePattern& type,
                              FavoriteStateList* list) const;
 
   // 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;
 
   // Request a scan if not scanning and run |callback| when the Scanning state
   // for any Device of network type |type| completes.
   void WaitForScan(const std::string& type, const base::Closure& callback);
 
   // Request a network scan then signal Shill to connect to the best available
   // networks when completed.
@@ skipping 93 matching lines @@
   virtual void DefaultNetworkServiceChanged(
       const std::string& service_path) OVERRIDE;
 
   // Called after construction. Called explicitly by tests after adding
   // test observers.
   void InitShillPropertyHandler();
 
  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);
 
   // NetworkState specific method for UpdateManagedStateProperties which
   // notifies observers.
   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();
 
   // Non-const getters for managed entries. These are const so that they can
   // be called by Get[Network|Device]State, even though they return non-const
   // pointers.
   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;
 
   // Gets the list specified by |type|.
   ManagedStateList* GetManagedList(ManagedState::ManagedType type);
 
   // Helper function to notify observers. Calls CheckDefaultNetworkChanged().
   void OnNetworkConnectionStateChanged(NetworkState* network);
 
   // Notifies observers when the default network or its properties change.
@@ skipping 32 matching lines @@
 
   // Keeps track of the default network for notifying observers when it changes.
   std::string default_network_path_;
 
   // List of interfaces on which portal check is enabled.
   std::string check_portal_list_;
 
   // 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);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_NETWORK_STATE_HANDLER_H_