Add chromeos::NetworkStateManager

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 <list>

[gauravsh, 2012/10/25 23:41:05]

Is this used anywhere?

[stevenjb, 2012/10/26 21:36:39]

Fixed

+#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"
+
+namespace base {
+class DictionaryValue;
+class ListValue;
+class Value;
+}
+
+namespace chromeos {
+
+class DeviceState;
+class NetworkState;
+class NetworkStateHandlerObserver;
+class ShillServiceObserver;
+class ShillManagerClient;
+
+// Class for tracking the list of visible networks and their state.
+//
+// This class maps essential state from the connection manager (Shill) for
+// each visible network. It is not used to change the state of services or
+// devices, only global (manager) state.
+//
+// All getters return the currently cached state. This class is expected to
+// keep states up to date by managing the appropriate Shill observers.
+// It will send invoke its own more specific observer methods when the

[gauravsh, 2012/10/25 23:41:05]

send invoke -> invoke

[stevenjb, 2012/10/26 21:36:39]

Done.

+// specified changes occur.
+//
+// This class must not outlive the ShillManagerClient instance.
+class CHROMEOS_EXPORT NetworkStateHandler
+    : public ShillPropertyChangedObserver {
+ public:
+  typedef std::vector<ManagedState*> ManagedStateList;
+  typedef std::vector<NetworkState*> NetworkStateList;
+  typedef std::vector<DeviceState*> DeviceStateList;
+
+  NetworkStateHandler();
+  ~NetworkStateHandler();
+
+  // Initialize NetworkStateHandler. Sends an initial property request.
+  void Init();
+
+  // Add remove observers.

[gauravsh, 2012/10/25 23:41:05]

NIT: Add/remove

[stevenjb, 2012/10/26 21:36:39]

Done.

+  void AddObserver(NetworkStateHandlerObserver* observer);
+  void RemoveObserver(NetworkStateHandlerObserver* observer);
+
+  // Returns true if |technology| is enabled / available.
+  bool TechnologyAvailable(const std::string& technology) const;
+  bool TechnologyEnabled(const std::string& technology) const;
+
+  // Asynchronously sets the enabled state for |technology|.
+  // Note: Modifes Manager state. TODO(stevenjb): Add a completion callback.
+  void SetTechnologyEnabled(const std::string& technology, bool enabled);
+
+  // Finds and returns a device by |path|. Returns NULL if not found.
+  const DeviceState* GetDeviceState(const std::string& path) const;

[gauravsh, 2012/10/25 23:41:05]

Suggest calling this device_path to distinguish between service and device
paths.

[stevenjb, 2012/10/26 21:36:39]

Done.

+
+  // Finds and returns a device by |type|. Returns NULL if not found.

[gauravsh, 2012/10/25 23:41:05]

device -> device state

[stevenjb, 2012/10/26 21:36:39]

Done.

+  const DeviceState* GetDeviceStateByType(const std::string& type) const;
+
+  // Finds and returns a network by |path|. Returns NULL if not found.

[gauravsh, 2012/10/25 23:41:05]

network state

[stevenjb, 2012/10/26 21:36:39]

Done.

+  // Note: NetworkState is frequently updated asynchronously, i.e. properties
+  // are not always updated all at once. This will contain the most recent
+  // value for each state. To receive notifications when the state changes,
+  // observer this class and implement NetworkServicePropertyChanged().
+  const NetworkState* GetNetworkState(const std::string& path) const;
+
+  // Returns the "active" network (first network in the list if connected),
+  // NULL if none.
+  const NetworkState* ActiveNetwork() const;
+
+  // Returns the first connected network of type |type|, otherwise NULL.
+  // An empty type will return any connected non-ethernet network.

[gauravsh, 2012/10/25 23:41:05]

Why do we need to special-case this to only return non-ethernet networks? Why
not just return any connected network?

[stevenjb, 2012/10/26 21:36:39]

Oops, not true, comment copied from connecting.

+  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.

[gauravsh, 2012/10/25 23:41:05]

See question about special-casing ethernet.

[stevenjb, 2012/10/26 21:36:39]

Ethernet is rarely if ever in a "connecting" state; this avoids occasionally
briefly flashing a connecting ethernet icon.

[gauravsh, 2012/10/27 00:26:33]

But why not handle this damping at the UI layer insted of hardcoding here?

+  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;

[gauravsh, 2012/10/26 01:15:01]

I don't quite understand why we need this. Isn't this information that can
already be derived indirectly from NetworkState returned by
ConnectedNetworkByType? I'd much rather prefer to keep the API simple even if
that means the consumers have to do a little bit of extra work.

And if this is only meant to return for connected networks, that should be
reflected in the name.

[stevenjb, 2012/10/26 21:36:39]

I improved the names, but feel that:
a) We will be calling this in several places in the UI
b) We only ever care about connected devices except perhaps in an advanced
device inspection view (which I don't think we currently have). It's in the
comment, adding that to the name feels awkward.
c) The hardware address is stored in the device. Having the UI do the network +
device lookup seems like it puts too much burden in the UI code.

+  // Same as above but in aa:bb format.
+  std::string HardwareAddressFormatted(const std::string& type) const;

[gauravsh, 2012/10/26 01:15:01]

Since everything is a string - please change type->technology_type to make that
clearer.

[stevenjb, 2012/10/26 21:36:39]

I think that would be more confusing. We use 'type' consistently to
differentiate between technologies. Converting that to technology_type
everywhere seems unnecessarily verbose.

+
+  // Returns the list of networks. This also sends a scan request to Shill.
+  const NetworkStateList& GetNetworkList() const;

[gauravsh, 2012/10/25 23:41:05]

Is this synchronous? Are the list of networks returned *after* Shill has
completed the scan? I'd clarify here.

[stevenjb, 2012/10/26 21:36:39]

Comments here have been expanded.

+
+  // ShillPropertyChangedObserver overrides.
+  virtual void OnPropertyChanged(const std::string& key,
+                                 const base::Value& value) OVERRIDE;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest, NetworkStateHandlerStub);
+
+  typedef std::map<std::string, ShillServiceObserver*> ShillServiceObserverMap;
+
+  // 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& path) const;
+  NetworkState* GetModifiableNetworkState(const std::string& path) const;
+
+  // 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);

[gauravsh, 2012/10/25 23:41:05]

Should this just be called SetAvailableTechnologies - Update implies either that
it is asking Shill to do something, or that it merging in newly received
information.

[stevenjb, 2012/10/26 21:36:39]

It's named update because it's more involved than a typical setter, but not much
more. Renamed.

+
+  // 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,

[gauravsh, 2012/10/25 23:41:05]

NIT: service_path

[stevenjb, 2012/10/26 21:36:39]

Done.

+                                     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& service_path,

[gauravsh, 2012/10/25 23:41:05]

I think it would be useful to group the ordering of these declearations such
that all Shill callbacks are in a single place, and so on. I see 3 types of
methods:
1) Things that update internal state directly.
2) Things that update internal state in response to a shill callback being
finished.
3) Things that ask shill to do something (and presumably return the result in
the form of a call back).

[stevenjb, 2012/10/26 21:36:39]

This has been refactored

+                           DBusMethodCallStatus call_status,
+                           const base::DictionaryValue& properties);
+
+  // Request an immediate network scan.
+  void RequestScan() const;
+
+  // Test accessors
+  int NumObservedNetworksForTest() const { return observed_networks_.size(); }
+
+  // Convenience pointer for ShillManagerClient
+  ShillManagerClient* shill_manager_;
+
+  // Observer list
+  ObserverList<NetworkStateHandlerObserver> observers_;
+
+  // Lists of managed states
+  NetworkStateList network_list_;
+  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
+  ShillServiceObserverMap 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<NetworkStateHandler> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_NETWORK_STATE_HANDLER_H_