Add chromeos::NetworkStateManager

chromeos/network/network_state_manager.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_MANAGER_H_
+#define CHROMEOS_NETWORK_NETWORK_STATE_MANAGER_H_
+
+#include <list>
+#include <map>
+#include <set>
+#include <string>
+
+#include "base/gtest_prod_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/dbus/shill_property_changed_observer.h"
+#include "chromeos/network/managed_state.h"
+
+// Class for tracking the list of visible networks and their state.

[Greg Spencer (Chromium), 2012/10/23 23:19:10]

Move down to class declaration.

[stevenjb, 2012/10/25 00:41:58]

Done.

+// This class maps essential state from the connection manager (Shill) for
+// each visible network. It is not used to change state, and should not be

[pneubeck (no reviews), 2012/10/24 14:41:42]

Actually, SetTechnologyEnabled does change state.
Please adapt the comment and emphasize at each state-changing method that it
does so.

[stevenjb, 2012/10/25 00:41:58]

Done.

+// used to track local state for networks.
+// This class must not outlive the ShillManagerClient instance.

[pneubeck (no reviews), 2012/10/24 14:41:42]

Maybe give also a comment on what the class guarantees and what not:
- Events/State changes may be missed (?)
- State getters are non-blocking and provide the last seen (maybe outdated)
state.

[stevenjb, 2012/10/25 00:41:58]

We will ensure this is not true.
Comments changed.

+
+namespace base {
+class DictionaryValue;
+class ListValue;
+class Value;
+}
+
+namespace chromeos {
+
+class DeviceState;
+class NetworkState;
+class NetworkServiceObserver;
+class ShillManagerClient;
+
+class CHROMEOS_EXPORT NetworkStateManager

[stevenjb, 2012/10/23 22:59:38]

Per discussion, will rename this NetworkStateHandler

[stevenjb, 2012/10/25 00:41:58]

Done.

+    : public ShillPropertyChangedObserver {
+ public:
+  typedef std::list<ManagedState*> ManagedStateList;
+  typedef std::list<NetworkState*> NetworkStateList;
+  typedef std::list<DeviceState*> DeviceStateList;
+
+  // Observer class for all network state changes, including changes to
+  // active (connecting or connected) services.
+  class Observer {

[Greg Spencer (Chromium), 2012/10/23 23:19:10]

It may be better to move this outside of the class and put in a separate file
for the reasons described at:

http://code.google.com/p/chromium/issues/detail?id=141261

Which seems to outline the "right" way to do this.

[stevenjb, 2012/10/25 00:41:58]

Done.

+   public:

[pneubeck (no reviews), 2012/10/24 14:41:42]

You could disallow copy/assign here, otherwise there will never be compiler
errors about copies/assigns to references of type Observer.
Then an empty constructor is also required.

[stevenjb, 2012/10/25 00:41:58]

Done

+    virtual ~Observer() {}
+
+    // If |property| is non empty than that property has changed. Otherwise
+    // multiple properties may have changed.
+    virtual void NetworkManagerChanged(const std::string& property) {}
+    // The list of networks changed.
+    virtual void NetworkListChanged(const NetworkStateList& networks) {}
+    // The active network changed. |network| will be NULL if there is no longer
+    // an active network.
+    virtual void ActiveNetworkChanged(const NetworkState* network) {}
+    // The state of the active network changed.
+    virtual void ActiveNetworkStateChanged(const NetworkState* network) {}
+    // A network service property changed. |property| is the name of the changed
+    // property (from service_constants.h). Note: for the active network,
+    // this will be called in *addition* to ActiveNetworkStateChanged() if
+    // property == flimflam::kStateProperty.
+    virtual void NetworkServicePropertyChanged(const NetworkState* network,
+                                               const std::string& property) {}
+  };
+
+  NetworkStateManager();
+  ~NetworkStateManager();
+
+  // Initialize NetworkStateManager. Sends an initial property request.
+  void Init();
+
+  // Add remove observers.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Request an immediate network scan.
+  void RequestScan() const;

[stevenjb, 2012/10/23 23:12:09]

Will remove and make implicit in GetNetworkList().

[stevenjb, 2012/10/25 00:41:58]

Done.

+
+  // Manage the state of enabled / available technologies.

[pneubeck (no reviews), 2012/10/24 14:41:42]

A bit misleading. Better split into separate comments for enabled and available.

+  bool TechnologyAvailable(const std::string& technology) const;
+  bool TechnologyEnabled(const std::string& technology) const;
+  void SetTechnologyEnabled(const std::string& technology, bool enabled);

[stevenjb, 2012/10/23 23:12:09]

Add TODO: Add an error handler callback for SetTechnologyEnabled. 
This is the only setter here and I don't think we need error handling for
getters. I think that we should develop a standard error callback when we add
NetworkConnectionHandler, then use that here (and elsewhere).

[pneubeck (no reviews), 2012/10/24 14:41:42]

Emphasize, that this does not change which technologies are tracked, but
actually tells shill which technologies to use. (perhaps a reference to
manager-api.txt is sufficient).

I thought you wanted to expose the asynchronous calls up to the UI (e.g. webui)?
But SetTechnologyEnabled doesn't notify about success/failure.

[Greg Spencer (Chromium), 2012/10/24 22:27:42]

As a general rule, how will we know about DBus call/Shill failures for the
methods that return cached data?  For instance, if you can't get device state
because the (earlier cache-filling) shill call failed for some reason, how will
I know what the error is at the UI level?

[stevenjb, 2012/10/25 00:41:58]

It is the responsibility of this class to maintain accurate state. If there are
failures communicating with Shill it should handle those and log them. I'm not
sure there is any real value to adding an "unknown" state for things like
Technlogy[Enabled|Available].

[Greg Spencer (Chromium), 2012/10/25 21:51:42]

On 2012/10/25 00:41:58, stevenjb (chromium) wrote: 
Well, I was more thinking about errors returned by Shill. Perhaps there should
be an error return for the things that can have errors, and that if there was an
error fetching things earlier, then we just give out the last error that
occurred, if any, and reset it when we succeed or get a new failure.

But I guess most of the errors that could be generated in this class are going
to be communication errors on DBus instead of Shill API errors, so it's probably
OK to just let the class deal with it internally.

+
+  // Finds and returns a device by |path|. Returns NULL if not found.
+  const DeviceState* GetDeviceState(const std::string& path) const;
+
+  // Finds and returns a device by |type|. Returns NULL if not found.
+  const DeviceState* GetDeviceStateByType(const std::string& type) const;
+
+  // Finds and returns a network by |path|. Returns NULL if not found.
+  const NetworkState* GetNetworkState(const std::string& path) const;
+
+  // Returns the "active" network (first network in the list if connected).

[Greg Spencer (Chromium), 2012/10/25 21:51:42]

What does this return if not connected? (I'm assuming NULL, but it's not
documented)

[stevenjb, 2012/10/25 22:05:22]

Done.

+  const NetworkState* ActiveNetwork() const;
+
+  // Returns the first connected network of type |type|, otherwise NULL.
+  // An empty type will return any connected non-ethernet network.
+  const NetworkState* ConnectedNetworkByType(const std::string& type) const;
+
+  // Returns the first connecting network of type |type|, otherwise NULL.
+  // An empty type will return any connecting non-ethernet network.
+  const NetworkState* ConnectingNetworkByType(const std::string& type) const;
+
+  // Returns the hardware (MAC) address for the first connected network
+  // matching |type|, or an empty string if none.
+  std::string HardwareAddress(const std::string& type) const;
+  // Same as above but in aa:bb format.
+  std::string HardwareAddressFormatted(const std::string& type) const;
+
+  const NetworkStateList& network_list() const { return network_list_; }

[stevenjb, 2012/10/23 23:12:09]

Will change to GetNetworkList() and have this request a scan (which will
eventually be automatic in Shill)

[stevenjb, 2012/10/25 00:41:58]

Done.

+
+  // ShillPropertyChangedObserver overrides.
+  virtual void OnPropertyChanged(const std::string& key,
+                                 const base::Value& value);

[Greg Spencer (Chromium), 2012/10/23 23:19:10]

Add OVERRIDE

[stevenjb, 2012/10/25 00:41:58]

Done.

+
+private:

[pneubeck (no reviews), 2012/10/24 14:41:42]

Since this is a rather central manager. Should the implementation perhaps be put
completely into the .cc file to avoid recompilation on implementation changes?

[stevenjb, 2012/10/25 00:41:58]

In general in Chrome we rely on fast compilation tools rather than
implementation abstractions. I don't expect that there will be frequent header
changes once initial development completes.

+  FRIEND_TEST_ALL_PREFIXES(NetworkStateManagerTest, NetworkStateManagerStub);
+
+  typedef std::map<std::string, NetworkServiceObserver*>
+      NetworkServiceObservers;

[Greg Spencer (Chromium), 2012/10/23 23:19:10]

nit: NetworkServiceObserverMap instead?

[stevenjb, 2012/10/25 00:41:58]

Done.

+
+  // Non-const getters for managed entries.
+  DeviceState* GetModifiableDeviceState(const std::string& path);

[pneubeck (no reviews), 2012/10/24 14:41:42]

Make these two methods const (using const_iterator), then the const_cast should
be unnecessary.

[stevenjb, 2012/10/25 00:41:58]

The const cast is for the return values, to avoid duplicate code. They would
still be required if these methods where const. It is generally preferred (at
least by Ben) that classes returning modifiable members are not const.

+  NetworkState* GetModifiableNetworkState(const std::string& path);
+
+  // Callback for dbus method fetching properties.
+  void ManagerPropertiesCallback(DBusMethodCallStatus call_status,
+                                 const base::DictionaryValue& properties);
+  // Called form OnPropertyChanged() and ManagerPropertiesCallback().
+  // Returns true if observers should be notified.
+  bool ManagerPropertyChanged(const std::string& key, const base::Value& value);
+
+  // Casts the list specified by |type| to a list of base ManagedState pointers.
+  ManagedStateList* GetManagedList(ManagedState::ManagedType type);
+
+  // Adds new entries to the managed list specified by |type| and deletes any
+  // entries that are no longer in the list.
+  void UpdateManagedList(ManagedState::ManagedType type,
+                         const base::ListValue& entries);
+
+  // Updates the Shill service property observers to observe any connected
+  // or connecting networks.
+  void UpdateObservedNetworkServices();
+
+  // Sets |available_technologies_| to contain only entries in |technologies|.
+  void UpdateAvailableTechnologies(const base::ListValue& technologies);
+
+  // Sets |enabled_technologies_| to contain only entries in |technologies|.
+  void UpdateEnabledTechnologies(const base::ListValue& technologies);
+
+  // Requests all properties for the service or device (called for new items).
+  void RequestProperties(ManagedState::ManagedType type,
+                         const std::string& path);
+
+  // Called when Shill returns the properties for a service or device.
+  void GetPropertiesCallback(ManagedState::ManagedType type,
+                             const std::string& path,
+                             DBusMethodCallStatus call_status,
+                             const base::DictionaryValue& properties);
+
+  // Parses the properties for the service or device. Mostly calls
+  // managed->PropertyChanged(key, value) for each dictionary entry.
+  void ParseProperties(ManagedState* managed,
+                       const base::DictionaryValue& properties);
+
+  // Callback invoked when a watched service property changes. Calls
+  // network->PropertyChanged(key, value) and signals observers.
+  void NetworkServicePropertyChanged(const std::string& path,
+                                     const std::string& key,
+                                     const base::Value& value);
+
+  // Callback for getting the IPConfig property of a Network. Handled here
+  // instead of in NetworkState so that all asynchronous requests are done
+  // in a single place (also simplifies NetworkState considerably).
+  void GetIPConfigCallback(const std::string& path,
+                           DBusMethodCallStatus call_status,
+                           const base::DictionaryValue& properties);
+
+  // Test accessors
+  int NumObservedNetworksForTest() const { return observed_networks_.size(); }
+
+  // Convenince pointer for ShillManagerClient

[pneubeck (no reviews), 2012/10/24 14:41:42]

nit: Convenience

[stevenjb, 2012/10/25 00:41:58]

Done.

+  ShillManagerClient* shill_manager_;
+
+  // Observer list
+  ObserverList<Observer> observers_;
+
+  // Lists of managed states
+  NetworkStateList network_list_;

[pneubeck (no reviews), 2012/10/24 14:41:42]

These lists are never modified, but only new ones created by UpdateManagedList.
Why not use vector<>? Or even scoped_vector<> to ensure correct deletion and
document ownership?

[stevenjb, 2012/10/25 00:41:58]

Evolutionary artifact. scoped_vector<> is high overhead, but vector<> is fine.

+  DeviceStateList device_list_;
+
+  // Pending update count for each managed state type
+  std::map<ManagedState::ManagedType, int> pending_updates_;
+
+  // Lists of available / enabled technologies
+  std::set<std::string> available_technologies_;
+  std::set<std::string> enabled_technologies_;
+
+  // List of network services with Shill property changed observers
+  NetworkServiceObservers observed_networks_;
+
+  // Keep track of the active network for notifying observers when it changes.
+  std::string active_network_path_;
+
+  // For Shill client callbacks
+  base::WeakPtrFactory<NetworkStateManager> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkStateManager);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_STATE_MANAGER_H_