network_health: rework mojo service structure

- Create a new structure to hold all all of the network health state.
- Add a call to the mojo service to return this structure on request.
- Merge the Device mojo struct into the Network struct.
- Remove GetActiveNetwork API call in favor of a single GetNetworks
  call.

Bug: chromium:1076177
Change-Id: I644348b85584e3dfda2e2406186fc77f03d9556b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2174172
Commit-Queue: Trent Begin <tbegin@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771115}

chrome/browser/chromeos/net/mojom/network_health.mojom

@@ -2,32 +2,64 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// API intended for retrieving a snapshot of the network health state. Returns
+// properties of any available network technologies and any connected or
+// connecting network when available.
+
 module chromeos.network_health.mojom;
 
 import "chromeos/services/network_config/public/mojom/network_types.mojom";
 
-// Contains information for a single network.
+enum NetworkState {
+  // The network type is available but not yet initialized.
+  kUninitialized,
+  // The network type is available but disabled.
+  kDisabled,
+  // The network type is available and enabling.
+  kEnabling,
+  // The network type is prohibited by policy.
+  kProhibited,
+  // The network type is available and enabled, but no network connection has
+  // been established.
+  kNotConnected,
+  // The network type is available and enabled, and a network connection is
+  // in progress.
+  kConnecting,
+  // The network is in a portal state.
+  kPortal,
+  // The network is in a connected state, but connectivity is limited.
+  kConnected,
+  // The network is connected and online.
+  kOnline,
+};
+
+// Contains information for a single network connection and underlying
+// network technology e.g WiFi.
 struct Network {
-  chromeos.network_config.mojom.ConnectionStateType connection_state;
-  string name;
+
   chromeos.network_config.mojom.NetworkType type;
+  NetworkState state;
+  // The user facing name of the network if available.
+  string? name;
+  // Optional string for the network's mac_address.
+  string? mac_address;
 };
 
-// Contains information for a single network device.
-struct Device {
-  chromeos.network_config.mojom.DeviceStateType connection_state;
-  // Optional string for the device’s mac_address. Will only be populated if
-  // applicable to the device.
-  string? mac_address;
-  chromeos.network_config.mojom.NetworkType type;
+// Aggregate structure for all of the network health state.
+struct NetworkHealthState {
+  // Networks will be sorted with active connections listed first.
+  array<Network> networks;
 };
 
 // Interface for retrieving aggregated information about the current network
 // state and health.
 interface NetworkHealthService {
-  // Returns a list of the network devices.
-  GetDeviceList() => (array<Device> devices);
+  // Returns a list of all the networks. Networks will be sorted with active
+  // connections listed first.
+  GetNetworkList() => (array<Network> networks);
 
-  // Returns a list of the active (connected or connecting) networks.
-  GetActiveNetworkList() => (array<Network> active_networks);
+  // Returns the current network health state.
+  // This is currently the same information provided by GetNetworkList. More
+  // information will be added over time.
+  GetHealthSnapshot() => (NetworkHealthState state);
 };

chrome/browser/chromeos/net/network_health.cc

@@ -14,9 +14,65 @@
 namespace chromeos {
 namespace network_health {
 
-NetworkHealth::NetworkHealthState::NetworkHealthState() {}
+namespace {
+
+constexpr mojom::NetworkState DeviceStateToNetworkState(
+    network_config::mojom::DeviceStateType device_state) {
+  switch (device_state) {
+    case network_config::mojom::DeviceStateType::kUninitialized:
+      return mojom::NetworkState::kUninitialized;
+    case network_config::mojom::DeviceStateType::kDisabled:
+      return mojom::NetworkState::kDisabled;
+    case network_config::mojom::DeviceStateType::kEnabling:
+      return mojom::NetworkState::kEnabling;
+    case network_config::mojom::DeviceStateType::kEnabled:
+      return mojom::NetworkState::kNotConnected;
+    case network_config::mojom::DeviceStateType::kProhibited:
+      return mojom::NetworkState::kProhibited;
+    case network_config::mojom::DeviceStateType::kUnavailable:
+      NOTREACHED();
+      return mojom::NetworkState::kUninitialized;
+  }
+}
+
+constexpr mojom::NetworkState ConnectionStateToNetworkState(
+    network_config::mojom::ConnectionStateType connection_state) {
+  switch (connection_state) {
+    case network_config::mojom::ConnectionStateType::kOnline:
+      return mojom::NetworkState::kOnline;
+    case network_config::mojom::ConnectionStateType::kConnected:
+      return mojom::NetworkState::kConnected;
+    case network_config::mojom::ConnectionStateType::kPortal:
+      return mojom::NetworkState::kPortal;
+    case network_config::mojom::ConnectionStateType::kConnecting:
+      return mojom::NetworkState::kConnecting;
+    case network_config::mojom::ConnectionStateType::kNotConnected:
+      return mojom::NetworkState::kNotConnected;
+  }
+}
+
+// Populates a mojom::NetworkPtr based on the given |device_prop| and
+// |network_prop| if a valid Network can be created. Returns a base::nullopt
+// otherwise. This function assumes that |device_prop| is populated, while
+// |network_prop| could be null.
+mojom::NetworkPtr CreateNetwork(
+    const network_config::mojom::DeviceStatePropertiesPtr& device_prop,
+    const network_config::mojom::NetworkStatePropertiesPtr& net_prop) {
+  auto net = mojom::Network::New();
+  net->mac_address = device_prop->mac_address;
+  net->type = device_prop->type;
+
+  if (net_prop) {
+    net->state = ConnectionStateToNetworkState(net_prop->connection_state);
+    net->name = net_prop->name;
+  } else {
+    net->state = DeviceStateToNetworkState(device_prop->device_state);
+  }
+
+  return net;
+}
 
-NetworkHealth::NetworkHealthState::~NetworkHealthState() {}
+}  // namespace
 
 NetworkHealth::NetworkHealth() {
   network_config::BindToInProcessInstance(
@@ -28,64 +84,98 @@ NetworkHealth::NetworkHealth() {
 
 NetworkHealth::~NetworkHealth() {}
 
-const NetworkHealth::NetworkHealthState&
-NetworkHealth::GetNetworkHealthState() {
-  NET_LOG(EVENT) << "Network Health State Requested";
-  return network_health_state_;
+void NetworkHealth::CreateNetworkHealthState() {
+  // If the device information has not been collected, the NetworkHealthState
+  // cannot be created.
+  if (device_properties_.size() == 0)
+    return;
+
+  network_health_state_.networks.clear();
+
+  std::unordered_map<network_config::mojom::NetworkType,
+                     network_config::mojom::DeviceStatePropertiesPtr>
+      device_type_map;
+
+  // This function only supports one Network structure per underlying device. If
+  // this assumption changes, this function will need to be reworked.
+  for (const auto& d : device_properties_) {
+    device_type_map[d->type] = mojo::Clone(d);
+  }
+
+  // For each NetworkStateProperties, create a Network structure using the
+  // underlying DeviceStateProperties. Remove devices from the type map that
+  // have an associated NetworkStateProperties.
+  for (const auto& net_prop : network_properties_) {
+    auto device_iter = device_type_map.find(net_prop->type);
+    if (device_iter == device_type_map.end()) {
+      continue;
+    }
+    network_health_state_.networks.push_back(
+        CreateNetwork(device_iter->second, net_prop));
+    device_type_map.erase(device_iter);
+  }
+
+  // For the remaining devices that do not have associated
+  // NetworkStateProperties, create Network structures.
+  for (const auto& device_prop : device_type_map) {
+    // Devices that have an kUnavailable state are not valid.
+    if (device_prop.second->device_state ==
+        network_config::mojom::DeviceStateType::kUnavailable) {
+      NET_LOG(ERROR) << "Device in unexpected state: "
+                     << device_prop.second->device_state;
+      continue;
+    }
+
+    network_health_state_.networks.push_back(
+        CreateNetwork(device_prop.second, nullptr));
+  }
 }
 
 void NetworkHealth::RefreshNetworkHealthState() {
-  RequestActiveNetworks();
+  RequestNetworkStateList();
   RequestDeviceStateList();
 }
 
-void NetworkHealth::GetDeviceList(GetDeviceListCallback callback) {
-  std::move(callback).Run(mojo::Clone(network_health_state_.devices));
+const mojom::NetworkHealthStatePtr NetworkHealth::GetNetworkHealthState() {
+  NET_LOG(EVENT) << "Network Health State Requested";
+  return network_health_state_.Clone();
 }
 
-void NetworkHealth::GetActiveNetworkList(
-    GetActiveNetworkListCallback callback) {
-  std::move(callback).Run(mojo::Clone(network_health_state_.active_networks));
+void NetworkHealth::GetNetworkList(GetNetworkListCallback callback) {
+  std::move(callback).Run(mojo::Clone(network_health_state_.networks));
 }
 
-void NetworkHealth::OnActiveNetworksReceived(
-    std::vector<network_config::mojom::NetworkStatePropertiesPtr>
-        network_props) {
-  std::vector<mojom::NetworkPtr> active_networks;
-  for (const auto& prop : network_props) {
-    active_networks.push_back(
-        mojom::Network::New(prop->connection_state, prop->name, prop->type));
-  }
-  network_health_state_.active_networks.swap(active_networks);
+void NetworkHealth::GetHealthSnapshot(GetHealthSnapshotCallback callback) {
+  std::move(callback).Run(network_health_state_.Clone());
+}
+
+void NetworkHealth::OnNetworkStateListReceived(
+    std::vector<network_config::mojom::NetworkStatePropertiesPtr> props) {
+  network_properties_.swap(props);
+  CreateNetworkHealthState();
 }
 
 void NetworkHealth::OnDeviceStateListReceived(
-    std::vector<network_config::mojom::DeviceStatePropertiesPtr> device_props) {
-  std::vector<mojom::DevicePtr> devices;
-  for (const auto& prop : device_props) {
-    devices.push_back(mojom::Device::New(
-        prop->device_state, prop->mac_address.value_or(""), prop->type));
-  }
-  network_health_state_.devices.swap(devices);
+    std::vector<network_config::mojom::DeviceStatePropertiesPtr> props) {
+  device_properties_.swap(props);
+  CreateNetworkHealthState();
 }
 
-void NetworkHealth::OnActiveNetworksChanged(
-    std::vector<network_config::mojom::NetworkStatePropertiesPtr>
-        network_props) {
-  OnActiveNetworksReceived(std::move(network_props));
+void NetworkHealth::OnNetworkStateListChanged() {
+  RequestNetworkStateList();
 }
 
 void NetworkHealth::OnDeviceStateListChanged() {
   RequestDeviceStateList();
 }
 
-void NetworkHealth::RequestActiveNetworks() {
+void NetworkHealth::RequestNetworkStateList() {
   remote_cros_network_config_->GetNetworkStateList(
       network_config::mojom::NetworkFilter::New(
           network_config::mojom::FilterType::kActive,
           network_config::mojom::NetworkType::kAll,
           network_config::mojom::kNoLimit),
-      base::BindOnce(&NetworkHealth::OnActiveNetworksReceived,
+      base::BindOnce(&NetworkHealth::OnNetworkStateListReceived,
                      base::Unretained(this)));
 }
 

chrome/browser/chromeos/net/network_health.h

@@ -18,25 +18,15 @@ namespace network_health {
 class NetworkHealth : public mojom::NetworkHealthService,
                       public network_config::mojom::CrosNetworkConfigObserver {
  public:
-  // Structure containing the current snapshot of the state of Network Health.
-  struct NetworkHealthState {
-    NetworkHealthState();
-    NetworkHealthState(const NetworkHealthState&) = delete;
-    ~NetworkHealthState();
-
-    std::vector<mojom::NetworkPtr> active_networks;
-    std::vector<mojom::DevicePtr> devices;
-  };
-
   NetworkHealth();
 
   ~NetworkHealth() override;
 
   // Returns the current NetworkHealthState.
-  const NetworkHealthState& GetNetworkHealthState();
+  const mojom::NetworkHealthStatePtr GetNetworkHealthState();
 
-  // Handler for receiving active networks.
-  void OnActiveNetworksReceived(
+  // Handler for receiving the network state list.
+  void OnNetworkStateListReceived(
       std::vector<network_config::mojom::NetworkStatePropertiesPtr>);
 
   // Handler for receiving networking devices.
@@ -44,25 +34,28 @@ class NetworkHealth : public mojom::NetworkHealthService,
       std::vector<network_config::mojom::DeviceStatePropertiesPtr>);
 
   // NetworkHealthService implementation
-  void GetDeviceList(GetDeviceListCallback) override;
-  void GetActiveNetworkList(GetActiveNetworkListCallback) override;
+  void GetNetworkList(GetNetworkListCallback) override;
+  void GetHealthSnapshot(GetHealthSnapshotCallback) override;
 
   // CrosNetworkConfigObserver implementation
-  void OnActiveNetworksChanged(
-      std::vector<network_config::mojom::NetworkStatePropertiesPtr>) override;
+  void OnNetworkStateListChanged() override;
   void OnDeviceStateListChanged() override;
 
   // CrosNetworkConfigObserver unimplemented callbacks
-  void OnNetworkStateListChanged() override {}
+  void OnActiveNetworksChanged(
+      std::vector<network_config::mojom::NetworkStatePropertiesPtr>) override {}
   void OnNetworkStateChanged(
       network_config::mojom::NetworkStatePropertiesPtr) override {}
   void OnVpnProvidersChanged() override {}
   void OnNetworkCertificatesChanged() override {}
 
  private:
+  // Creates the NetworkHealthState structure from cached network information.
+  void CreateNetworkHealthState();
+
   // Asynchronous call that refreshes the current Network Health State.
   void RefreshNetworkHealthState();
-  void RequestActiveNetworks();
+  void RequestNetworkStateList();
   void RequestDeviceStateList();
 
   mojo::Remote<network_config::mojom::CrosNetworkConfig>
@@ -70,7 +63,12 @@ class NetworkHealth : public mojom::NetworkHealthService,
   mojo::Receiver<network_config::mojom::CrosNetworkConfigObserver>
       cros_network_config_observer_receiver_{this};
 
-  NetworkHealthState network_health_state_;
+  mojom::NetworkHealthState network_health_state_;
+
+  std::vector<network_config::mojom::DeviceStatePropertiesPtr>
+      device_properties_;
+  std::vector<network_config::mojom::NetworkStatePropertiesPtr>
+      network_properties_;
 };
 
 }  // namespace network_health