Improve NetworkStateHandler 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 8c2d12573c62d13aa87ee8a15bcb0b3fbaddd134..8fe91341057c3bb7770e054b064de7fe19f88169 100644
--- a/chromeos/network/network_state_handler.h
+++ b/chromeos/network/network_state_handler.h
@@ -67,7 +67,7 @@ class CHROMEOS_EXPORT NetworkStateHandler
   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,
@@ -86,15 +86,16 @@ class CHROMEOS_EXPORT NetworkStateHandler
   // 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
@@ -186,6 +187,10 @@ class CHROMEOS_EXPORT NetworkStateHandler
                                    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_;
 
@@ -200,8 +205,8 @@ class CHROMEOS_EXPORT NetworkStateHandler
   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);
 };