Add NetworkStateHandler::GetActiveNetworkListByType and observer

This introduces:
* NetworkStateHandler::GetActiveNetworkListByType()
* NetworkStateHandler::ActiveNetworkByType()
* NetworkStateHandlerObserver::ActiveNetworksChanged()

For cros_network_config.mojo we are planning to use "active" (connected
or connecting) networks instead of "default" network (which can be a
VPN, and doesn't tell the whole story when, e.g. WiFi and Cellular are
both connected). This adds the concept to NetworkStateHandler and
implements it in two places where it is a straightforward improvement.

Bug: 862420
Change-Id: Ie49efd5d886f994cb24cd22bf361dd404450e96c
Reviewed-on: https://chromium-review.googlesource.com/c/1433113Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629179}

ash/system/network/tray_network_state_observer.cc

@@ -30,8 +30,7 @@ bool IsWifiEnabled() {
 namespace ash {
 
 TrayNetworkStateObserver::TrayNetworkStateObserver(Delegate* delegate)
-    : delegate_(delegate),
-      update_frequency_(kUpdateFrequencyMs) {
+    : delegate_(delegate), update_frequency_(kUpdateFrequencyMs) {
   if (ui::ScopedAnimationDurationScaleMode::duration_scale_mode() !=
       ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION) {
     update_frequency_ = 0;  // Send updates immediately for tests.
@@ -64,18 +63,8 @@ void TrayNetworkStateObserver::DeviceListChanged() {
   SignalUpdate(false /* notify_a11y */);
 }
 
-// Any change to the Default (primary connected) network, including Strength
-// changes, should trigger a NetworkStateChanged update.
-void TrayNetworkStateObserver::DefaultNetworkChanged(
-    const chromeos::NetworkState* network) {
-  SignalUpdate(true /* notify_a11y */);
-}
-
-// Any change to the Connection State should trigger a NetworkStateChanged
-// update. This is important when both a VPN and a physical network are
-// connected.
-void TrayNetworkStateObserver::NetworkConnectionStateChanged(
-    const chromeos::NetworkState* network) {
+void TrayNetworkStateObserver::ActiveNetworksChanged(
+    const std::vector<const chromeos::NetworkState*>& active_networks) {
   SignalUpdate(true /* notify_a11y */);
 }
 

ash/system/network/tray_network_state_observer.h

@@ -5,6 +5,8 @@
 #ifndef ASH_SYSTEM_NETWORK_TRAY_NETWORK_STATE_OBSERVER_H_
 #define ASH_SYSTEM_NETWORK_TRAY_NETWORK_STATE_OBSERVER_H_
 
+#include <vector>
+
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "chromeos/network/network_state_handler_observer.h"
@@ -33,9 +35,8 @@ class TrayNetworkStateObserver
   // NetworkStateHandlerObserver
   void NetworkListChanged() override;
   void DeviceListChanged() override;
-  void DefaultNetworkChanged(const chromeos::NetworkState* network) override;
-  void NetworkConnectionStateChanged(
-      const chromeos::NetworkState* network) override;
+  void ActiveNetworksChanged(const std::vector<const chromeos::NetworkState*>&
+                                 active_networks) override;
   void NetworkPropertiesUpdated(const chromeos::NetworkState* network) override;
 
   // NetworkPortalDetector::Observer

chrome/browser/ui/ash/network/network_state_notifier.cc

@@ -134,13 +134,15 @@ const char NetworkStateNotifier::kNetworkActivateNotificationId[] =
 const char NetworkStateNotifier::kNetworkOutOfCreditsNotificationId[] =
     "chrome://settings/internet/out-of-credits";
 
-NetworkStateNotifier::NetworkStateNotifier()
-    : did_show_out_of_credits_(false), weak_ptr_factory_(this) {
+NetworkStateNotifier::NetworkStateNotifier() : weak_ptr_factory_(this) {
   if (!NetworkHandler::IsInitialized())
     return;
   NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
   handler->AddObserver(this, FROM_HERE);
-  UpdateDefaultNetwork(handler->DefaultNetwork());
+  NetworkStateHandler::NetworkStateList active_networks;
+  handler->GetActiveNetworkListByType(NetworkTypePattern::Default(),
+                                      &active_networks);
+  ActiveNetworksChanged(active_networks);
   NetworkHandler::Get()->network_connection_handler()->AddObserver(this);
 }
 
@@ -158,7 +160,7 @@ void NetworkStateNotifier::ConnectToNetworkRequested(
       NetworkHandler::Get()->network_state_handler()->GetNetworkState(
           service_path);
   if (network && network->type() == shill::kTypeVPN)
-    connected_vpn_.clear();
+    connected_vpn_guid_.clear();
 
   RemoveConnectNotification();
 }
@@ -186,105 +188,113 @@ void NetworkStateNotifier::DisconnectRequested(
       NetworkHandler::Get()->network_state_handler()->GetNetworkState(
           service_path);
   if (network && network->type() == shill::kTypeVPN)
-    connected_vpn_.clear();
+    connected_vpn_guid_.clear();
 }
 
-void NetworkStateNotifier::DefaultNetworkChanged(const NetworkState* network) {
-  if (!UpdateDefaultNetwork(network))
-    return;
-  // If the default network changes to another network, allow the out of
-  // credits notification to be shown again. A delay prevents the notification
-  // from being shown too frequently (see below).
-  if (network)
+void NetworkStateNotifier::ActiveNetworksChanged(
+    const std::vector<const NetworkState*>& active_networks) {
+  std::string active_non_vpn_network_guid;
+  for (const auto* network : active_networks) {
+    if (network->type() == shill::kTypeVPN)
+      UpdateVpnConnectionState(network);
+    else if (active_non_vpn_network_guid.empty())
+      active_non_vpn_network_guid = network->guid();
+  }
+  // If the default network changes, allow the out of credits notification to be
+  // shown again. A delay prevents the notification from being shown too
+  // frequently (see below).
+  if (active_non_vpn_network_guid != active_non_vpn_network_guid_) {
+    active_non_vpn_network_guid_ = active_non_vpn_network_guid;
     did_show_out_of_credits_ = false;
-}
-
-void NetworkStateNotifier::NetworkConnectionStateChanged(
-    const NetworkState* network) {
-  if (network->type() == shill::kTypeVPN)
-    UpdateVpnConnectionState(network);
+    UpdateCellularOutOfCredits();
+  }
 }
 
 void NetworkStateNotifier::NetworkPropertiesUpdated(
     const NetworkState* network) {
   if (network->type() != shill::kTypeCellular)
     return;
-  UpdateCellularOutOfCredits(network);
+  if (network->cellular_out_of_credits())
+    UpdateCellularOutOfCredits();
   UpdateCellularActivating(network);
 }
 
-bool NetworkStateNotifier::UpdateDefaultNetwork(const NetworkState* network) {
-  std::string default_network_path;
-  if (network)
-    default_network_path = network->path();
-  if (default_network_path != last_default_network_) {
-    last_default_network_ = default_network_path;
-    return true;
-  }
-  return false;
-}
-
 void NetworkStateNotifier::UpdateVpnConnectionState(const NetworkState* vpn) {
-  if (vpn->path() == connected_vpn_) {
-    if (!vpn->IsConnectedState() && !vpn->IsConnectingState()) {
+  if (vpn->guid() == connected_vpn_guid_) {
+    if (!vpn->IsConnectingOrConnected()) {
       if (vpn->GetVpnProviderType() != shill::kProviderArcVpn) {
         ShowVpnDisconnectedNotification(vpn);
       }
-      connected_vpn_.clear();
+      connected_vpn_guid_.clear();
     }
   } else if (vpn->IsConnectedState()) {
-    connected_vpn_ = vpn->path();
+    connected_vpn_guid_ = vpn->guid();
   }
 }
 
-void NetworkStateNotifier::UpdateCellularOutOfCredits(
-    const NetworkState* cellular) {
-  // Only display a notification if we are out of credits and have not already
-  // shown a notification (or have since connected to another network type).
-  if (!cellular->cellular_out_of_credits() || did_show_out_of_credits_)
+void NetworkStateNotifier::UpdateCellularOutOfCredits() {
+  // Only show the notification once (reset when the primary network changes).
+  if (did_show_out_of_credits_)
     return;
 
-  // Only display a notification if not connected, connecting, or waiting to
-  // connect to another network.
-  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
-  const NetworkState* default_network = handler->DefaultNetwork();
-  if (default_network && default_network != cellular)
-    return;
-  if (handler->ConnectingNetworkByType(NetworkTypePattern::NonVirtual()) ||
-      NetworkHandler::Get()
+  // Don't display notification if the UI has requested a network connection.
+  if (NetworkHandler::Get()
           ->network_connection_handler()
-          ->HasPendingConnectRequest())
+          ->HasPendingConnectRequest()) {
     return;
+  }
+
+  NetworkStateHandler::NetworkStateList active_networks;
+  NetworkHandler::Get()->network_state_handler()->GetActiveNetworkListByType(
+      NetworkTypePattern::NonVirtual(), &active_networks);
+  const NetworkState* primary_network = nullptr;
+  for (const auto* network : active_networks) {
+    // Don't display notification if any network is connecting.
+    if (network->IsConnectingState())
+      return;
+    if (!primary_network)
+      primary_network = network;
+  }
+
+  if (!primary_network ||
+      !primary_network->Matches(NetworkTypePattern::Cellular()) ||
+      !primary_network->cellular_out_of_credits()) {
+    return;
+  }
 
   did_show_out_of_credits_ = true;
   base::TimeDelta dtime = base::Time::Now() - out_of_credits_notify_time_;
   if (dtime.InSeconds() > kMinTimeBetweenOutOfCreditsNotifySeconds) {
     out_of_credits_notify_time_ = base::Time::Now();
-    base::string16 error_msg = l10n_util::GetStringFUTF16(
-        IDS_NETWORK_OUT_OF_CREDITS_BODY, base::UTF8ToUTF16(cellular->name()));
+    base::string16 error_msg =
+        l10n_util::GetStringFUTF16(IDS_NETWORK_OUT_OF_CREDITS_BODY,
+                                   base::UTF8ToUTF16(primary_network->name()));
     ShowErrorNotification(
-        cellular->path(), kNetworkOutOfCreditsNotificationId, cellular->type(),
+        primary_network->path(), kNetworkOutOfCreditsNotificationId,
+        primary_network->type(),
         l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_TITLE), error_msg,
         base::Bind(&NetworkStateNotifier::ShowMobileSetup,
-                   weak_ptr_factory_.GetWeakPtr(), cellular->guid()));
+                   weak_ptr_factory_.GetWeakPtr(), primary_network->guid()));
   }
 }
 
 void NetworkStateNotifier::UpdateCellularActivating(
     const NetworkState* cellular) {
+  const std::string cellular_guid = cellular->guid();
   // Keep track of any activating cellular network.
   std::string activation_state = cellular->activation_state();
   if (activation_state == shill::kActivationStateActivating) {
-    cellular_activating_.insert(cellular->path());
+    cellular_activating_guids_.insert(cellular_guid);
     return;
   }
   // Only display a notification if this network was activating and is now
   // activated.
-  if (!cellular_activating_.count(cellular->path()) ||
-      activation_state != shill::kActivationStateActivated)
+  if (!cellular_activating_guids_.count(cellular_guid) ||
+      activation_state != shill::kActivationStateActivated) {
     return;
+  }
 
-  cellular_activating_.erase(cellular->path());
+  cellular_activating_guids_.erase(cellular_guid);
   std::unique_ptr<message_center::Notification> notification =
       ash::CreateSystemNotification(
           message_center::NOTIFICATION_TYPE_SIMPLE,
@@ -298,7 +308,7 @@ void NetworkStateNotifier::UpdateCellularActivating(
           {},
           new message_center::HandleNotificationClickDelegate(
               base::Bind(&NetworkStateNotifier::ShowNetworkSettings,
-                         weak_ptr_factory_.GetWeakPtr(), cellular->guid())),
+                         weak_ptr_factory_.GetWeakPtr(), cellular_guid)),
           ash::kNotificationMobileDataIcon,
           message_center::SystemNotificationWarningLevel::CRITICAL_WARNING);
   notification->set_priority(message_center::SYSTEM_PRIORITY);

chrome/browser/ui/ash/network/network_state_notifier.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
@@ -62,8 +63,8 @@ class NetworkStateNotifier : public NetworkConnectionObserver,
   void DisconnectRequested(const std::string& service_path) override;
 
   // NetworkStateHandlerObserver
-  void DefaultNetworkChanged(const NetworkState* network) override;
-  void NetworkConnectionStateChanged(const NetworkState* network) override;
+  void ActiveNetworksChanged(
+      const std::vector<const NetworkState*>& active_networks) override;
   void NetworkPropertiesUpdated(const NetworkState* network) override;
 
   void ConnectErrorPropertiesSucceeded(
@@ -89,7 +90,7 @@ class NetworkStateNotifier : public NetworkConnectionObserver,
 
   // Helper methods to update state and check for notifications.
   void UpdateVpnConnectionState(const NetworkState* vpn);
-  void UpdateCellularOutOfCredits(const NetworkState* cellular);
+  void UpdateCellularOutOfCredits();
   void UpdateCellularActivating(const NetworkState* cellular);
 
   // Shows the network settings for |network_id|.
@@ -98,11 +99,19 @@ class NetworkStateNotifier : public NetworkConnectionObserver,
   // Shows the mobile setup dialog for |network_id|.
   void ShowMobileSetup(const std::string& network_id);
 
-  std::string last_default_network_;
-  bool did_show_out_of_credits_;
+  // Set to the GUID of the connected VPN network if any, otherwise empty.
+  // Used for displaying VPN disconnected notification.
+  std::string connected_vpn_guid_;
+
+  // Tracks state for out of credits notification.
+  bool did_show_out_of_credits_ = false;
   base::Time out_of_credits_notify_time_;
-  std::set<std::string> cellular_activating_;
-  std::string connected_vpn_;
+  // Set to the GUID of the active non VPN network if any, otherwise empty.
+  std::string active_non_vpn_network_guid_;
+
+  // Tracks GUIDs of activating cellular networks for activation notification.
+  std::set<std::string> cellular_activating_guids_;
+
   base::WeakPtrFactory<NetworkStateNotifier> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkStateNotifier);

chromeos/network/network_state_handler.h

@@ -161,12 +161,15 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   // differ.
   const NetworkState* DefaultNetwork() const;
 
-  // Returns the primary connected network of matching |type|, otherwise NULL.
+  // Returns the primary connected network matching |type|, otherwise null.
   const NetworkState* ConnectedNetworkByType(const NetworkTypePattern& type);
 
-  // Like ConnectedNetworkByType() but returns a connecting network or NULL.
-  const NetworkState* ConnectingNetworkByType(
-      const NetworkTypePattern& type) const;
+  // Returns the primary connecting network matching |type|, otherwise null.
+  const NetworkState* ConnectingNetworkByType(const NetworkTypePattern& type);
+
+  // Returns the primary active network of matching |type|, otherwise null.
+  // See also GetActiveNetworkListByType.
+  const NetworkState* ActiveNetworkByType(const NetworkTypePattern& type);
 
   // Like ConnectedNetworkByType() but returns any matching visible network or
   // NULL. Mostly useful for mobile networks where there is generally only one
@@ -200,6 +203,14 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
                             size_t limit,
                             NetworkStateList* list);
 
+  // Sets |list| to contain the active networks matching |type|. An 'active'
+  // network is connecting or connected, and the first connected active network
+  // is the primary or 'default' network providing connectivity (which may be a
+  // VPN, use NetworkTypePattern::NonVirtual() to ignore VPNs). See
+  // GetNetworkListByType for notes on |list| results.
+  void GetActiveNetworkListByType(const NetworkTypePattern& type,
+                                  NetworkStateList* list);
+
   // Finds and returns the NetworkState associated with |service_path| or NULL
   // if not found. If |configured_only| is true, only returns saved entries
   // (IsInProfile is true).
@@ -452,6 +463,14 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest,
                            BlockedByPolicyOnlyManagedIfAvailable);
 
+  // Implementation for GetNetworkListByType and GetActiveNetworkListByType.
+  void GetNetworkListByTypeImpl(const NetworkTypePattern& type,
+                                bool configured_only,
+                                bool visible_only,
+                                bool active_only,
+                                size_t limit,
+                                NetworkStateList* list);
+
   // Sorts the network list. Called when all network updates have been received,
   // or when the network list is requested but the list is in an unsorted state.
   // Networks are sorted as follows, maintaining the existing relative ordering:
@@ -527,6 +546,11 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   // Notifies observers when the default network or its properties change.
   void NotifyDefaultNetworkChanged();
 
+  // Notifies observers when the active state of any current or previously
+  // active network changes, or the active networks order changes.
+  bool ActiveNetworksChanged(const NetworkStateList& active_networks);
+  void NotifyIfActiveNetworksChanged();
+
   // Notifies observers about changes to |network|, including IPConfg.
   void NotifyNetworkPropertiesUpdated(const NetworkState* network);
 
@@ -600,6 +624,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   // |network_list_|.
   ManagedStateList tether_network_list_;
 
+  // List of active networks, used to limit ActiveNetworksChanged events.
+  class ActiveNetworkState;
+  std::vector<ActiveNetworkState> active_network_list_;
+
   // Set to true when the network list is sorted, cleared when network updates
   // arrive. Used to trigger sorting when needed.
   bool network_list_sorted_ = false;

chromeos/network/network_state_handler_observer.cc

@@ -10,33 +10,29 @@ NetworkStateHandlerObserver::NetworkStateHandlerObserver() = default;
 
 NetworkStateHandlerObserver::~NetworkStateHandlerObserver() = default;
 
-void NetworkStateHandlerObserver::NetworkListChanged() {
-}
+void NetworkStateHandlerObserver::NetworkListChanged() {}
 
-void NetworkStateHandlerObserver::DeviceListChanged() {
-}
+void NetworkStateHandlerObserver::DeviceListChanged() {}
 
 void NetworkStateHandlerObserver::DefaultNetworkChanged(
-    const NetworkState* network) {
-}
+    const NetworkState* network) {}
 
 void NetworkStateHandlerObserver::NetworkConnectionStateChanged(
-    const NetworkState* network) {
-}
+    const NetworkState* network) {}
+
+void NetworkStateHandlerObserver::ActiveNetworksChanged(
+    const std::vector<const NetworkState*>& active_networks) {}
 
 void NetworkStateHandlerObserver::NetworkPropertiesUpdated(
-    const NetworkState* network) {
-}
+    const NetworkState* network) {}
 
 void NetworkStateHandlerObserver::DevicePropertiesUpdated(
-    const chromeos::DeviceState* device) {
-}
+    const chromeos::DeviceState* device) {}
 
 void NetworkStateHandlerObserver::ScanRequested(
     const NetworkTypePattern& type) {}
 
-void NetworkStateHandlerObserver::ScanCompleted(const DeviceState* device) {
-}
+void NetworkStateHandlerObserver::ScanCompleted(const DeviceState* device) {}
 
 void NetworkStateHandlerObserver::OnShuttingDown() {}
 

chromeos/network/network_state_handler_observer.h

@@ -41,6 +41,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandlerObserver {
   // The connection state of |network| changed.
   virtual void NetworkConnectionStateChanged(const NetworkState* network);
 
+  // Triggered when the connection state of any current or previously active
+  // (connected or connecting) network changes. Provides the current list of
+  // active networks, which may include a VPN.
+  virtual void ActiveNetworksChanged(
+      const std::vector<const NetworkState*>& active_networks);
+
   // One or more properties of |network| have been updated. Note: this will get
   // called in *addition* to NetworkConnectionStateChanged() when the
   // connection state property changes. Use this to track properties like