Improve NetworkStateHandler 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 49 matching lines @@
 
   // Add/remove observers.
   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. Calls |error_callback| on failure.
+  // Note: Modifies Manager state. Calls |error_callback| on failure.
   void SetTechnologyEnabled(
       const std::string& technology,
       bool enabled,
       const network_handler::ErrorCallback& error_callback);
 
   // Finds and returns a device state by |device_path| or NULL if not found.
   const DeviceState* GetDeviceState(const std::string& device_path) const;
 
   // Finds and returns a device state by |type|. Returns NULL if not found.
   const DeviceState* GetDeviceStateByType(const std::string& type) const;
 
   // Finds and returns a network state by |service_path| or NULL if not found.
   // 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 NetworkServiceChanged().
   const NetworkState* GetNetworkState(const std::string& service_path) const;
 
-  // Returns the "active" network (first network in the list if connected),
-  // NULL if none.
-  const NetworkState* ActiveNetwork() const;
+  // Returns the default connected network (which includes VPNs) or NULL.
+  // This is equivalent to ConnectedNetworkByType(kMatchTypeDefault).
+  const NetworkState* DefaultNetwork() const;
 
   // Returns the first connected network of type |type|, otherwise NULL.

[pneubeck (no reviews), 2012/12/19 15:22:38]

Do we assume that the first connected network has some special meaning/purpose,
because Shill sorts networks/services in a certain way?

If so, we should at least document that here. A lot of code will depend on that
function.

However, as explained to me by Darin Petkov, what Shill keeps track of is the
network through which all (or by default) traffic is routed. And that is
something completely different from only being connected.

E.g. what do we want to do if Ethernet, Wifi and Cellular are connected?
How do we decide what to show in the network menu icon in the tray menu?

Please, please ensure that this decision is never done on Chrome's side.

Currently it is done in NetworkLibrary::active_network() with a random heuristic
where wifi overwrites cellular. But that is simply wrong and easily out of sync
with the decisions Shill is taking.

[stevenjb, 2012/12/19 18:01:07]

Yes.
Replaced "first" with primary. Shill passes us the networks in the same priority
order that it uses, so "first" is really an implementation detail.
This decision is partly up to Shill and partly up to the UI. e.g. we don't want
to show any icon if the Default is ethernet, but do want to show an icon if we
are conneting to any Wireless. It is impossible to completely separate the two.
That is why I created these additional types so that the UI logic can be more
flexible and independent of the Shill implementation. 
The heuristic is not random. The purpose of the highest_priority() logic in
active_network() is designed to take that into account. However, that logic is
(clearly) complex and difficult to follow, which is why we are trying to
simplify it here.

On several occasions we have stumbled upon places where different parts of the
code want different results for active_network() which is why I am changing the
terminology and adding additional flexibility for the UI.

[pneubeck (no reviews), 2012/12/19 19:40:06]

As there are several users (uses in the code) of "active network" which also
have different expectations on what "active" means, that indicates that we
should either provide

A) a set of more basic functions which have to be combined by the users.
In that case, the functions need a very clear semantic.
For example, we could provide the list of all connected/connecting networks and
the interested caller has to sort/pick the right network on his own.

or
B) we could provide specifically tailored functions which solve each of the use
cases and their semantics cannot be explained easily. But then, we should
mention for which usage each function is meant for.
A really sketchy example:
 a) ... returns primary network which is the network that the ChromeOS user is
primarily interested in.
 b) ... returns the network with the current default route of the network
traffic. Used for proxy and network notifications of the browser.

+  // |type| can be a type defined in service_constants.h, or the additional
+  // match types defined in NetworkState; see NetworkState::MatchesType().
   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.
+  // Like ConnectedNetworkByType() but returns the connecting network or NULL.
   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 HardwareAddressForType(const std::string& type) const;
   // Same as above but in aa:bb format.
   std::string FormattedHardwareAddressForType(const std::string& type) const;
 
   // Sets |list| to contain the list of networks.  The returned list contains
   // a copy of NetworkState pointers which should not be stored or used beyond
@@ skipping 66 matching lines @@
   // pointers.
   DeviceState* GetModifiableDeviceState(const std::string& device_path) const;
   NetworkState* GetModifiableNetworkState(
       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 called to parse |network| properties.

[pneubeck (no reviews), 2012/12/19 15:22:38]

Please document this function more clearly.

Something like the follwing I'd guess:
Updates the property |key| of the stored NetworkState associated with |network|
to |value|. Returns whether |value| was different from the stored value."

[stevenjb, 2012/12/19 18:01:07]

I separated out the observer notification code. I also noticed a bug as I did so
- we want to be sure to only call the NetworkStateChanged() observer when an
actual change occurred; we don't care for the generic NetworkPropertyChanged().
Added a test to verify that.

   bool ParseNetworkServiceProperty(NetworkState* network,
                                    const std::string& key,
                                    const base::Value& value);
 
+  // Checks to see if the default network changed. Signals observers and
+  // returns true if it has.
+  bool CheckDefaultNetworkChanged();
+
   // Shill property handler instance, owned by this class.
   scoped_ptr<internal::ShillPropertyHandler> shill_property_handler_;
 
   // Observer list
   ObserverList<NetworkStateHandlerObserver> observers_;
 
   // Lists of managed states
   ManagedStateList network_list_;
   ManagedStateList device_list_;
 
   // Lists of available / enabled technologies
   std::set<std::string> available_technologies_;
   std::set<std::string> enabled_technologies_;
 
-  // Keeps track of the active network for notifying observers when it changes.
-  std::string active_network_path_;
+  // Keeps track of the default network for notifying observers when it changes.
+  std::string default_network_path_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_NETWORK_STATE_HANDLER_H_