Use mojo api in ash::NetworkFeaturePodController

This CL:
* Introduces mojom::CrosNetworkConfig::SetNetworkTypeEnabledState
  to replace the enable/disableNetworkType APIs in networkingPrivate
* Uses the API in NetworkFeaturePodController and removes the
  NetworkState dependencies.
* Uses TrayNetworkStateModel in VPNFeaturePodController and removes
  NetworkState dependencies.
* Cleans up some missing const qualifiers in TrayNetworkStateModel.

Bug: 862420
Change-Id: Ib43092dde79f9c80477234020a49751bc4c3620e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1623486Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662801}

ash/system/network/active_network_icon.cc

@@ -192,7 +192,7 @@ gfx::ImageSkia ActiveNetworkIcon::GetSingleImage(
     network_icon::IconType icon_type,
     bool* animating) {
   // If no network, check for cellular initializing.
-  NetworkStateProperties* default_network = model_->default_network();
+  const NetworkStateProperties* default_network = model_->default_network();
   if (!default_network && cellular_uninitialized_msg_ != 0) {
     if (animating)
       *animating = true;
@@ -205,7 +205,7 @@ gfx::ImageSkia ActiveNetworkIcon::GetSingleImage(
 gfx::ImageSkia ActiveNetworkIcon::GetDualImagePrimary(
     network_icon::IconType icon_type,
     bool* animating) {
-  NetworkStateProperties* default_network = model_->default_network();
+  const NetworkStateProperties* default_network = model_->default_network();
   if (default_network && default_network->type == NetworkType::kCellular) {
     if (chromeos::network_config::StateIsConnected(
             default_network->connection_state)) {
@@ -239,7 +239,7 @@ gfx::ImageSkia ActiveNetworkIcon::GetDualImageCellular(
         NetworkType::kCellular, icon_type);
   }
 
-  NetworkStateProperties* active_cellular = model_->active_cellular();
+  const NetworkStateProperties* active_cellular = model_->active_cellular();
   if (!active_cellular) {
     if (animating)
       *animating = false;
@@ -260,7 +260,7 @@ gfx::ImageSkia ActiveNetworkIcon::GetDefaultImageImpl(
     return GetDefaultImageForNoNetwork(icon_type, animating);
   }
   // Don't show connected Ethernet in the tray unless a VPN is present.
-  NetworkStateProperties* active_vpn = model_->active_vpn();
+  const NetworkStateProperties* active_vpn = model_->active_vpn();
   if (network->type == NetworkType::kEthernet && IsTrayIcon(icon_type) &&
       !active_vpn) {
     if (animating)
@@ -303,7 +303,8 @@ gfx::ImageSkia ActiveNetworkIcon::GetDefaultImageForNoNetwork(
 }
 
 void ActiveNetworkIcon::SetCellularUninitializedMsg() {
-  DeviceStateProperties* cellular = model_->GetDevice(NetworkType::kCellular);
+  const DeviceStateProperties* cellular =
+      model_->GetDevice(NetworkType::kCellular);
   if (cellular && cellular->state == DeviceStateType::kUninitialized) {
     cellular_uninitialized_msg_ = IDS_ASH_STATUS_TRAY_INITIALIZING_CELLULAR;
     uninitialized_state_time_ = base::Time::Now();

ash/system/network/network_feature_pod_button.cc

@@ -151,7 +151,7 @@ void NetworkFeaturePodButton::Update() {
 
   TrayNetworkStateModel* model =
       Shell::Get()->system_tray_model()->network_state_model();
-  NetworkStateProperties* network = model->default_network();
+  const NetworkStateProperties* network = model->default_network();
 
   bool toggled = network || model->GetDeviceState(NetworkType::kWiFi) ==
                                 DeviceStateType::kEnabled;

ash/system/network/network_feature_pod_controller.cc

@@ -8,15 +8,14 @@
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
+#include "ash/system/model/system_tray_model.h"
 #include "ash/system/network/network_feature_pod_button.h"
+#include "ash/system/network/tray_network_state_model.h"
 #include "ash/system/unified/unified_system_tray_controller.h"
-#include "chromeos/network/network_state.h"
-#include "chromeos/network/network_state_handler.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using chromeos::NetworkHandler;
-using chromeos::NetworkTypePattern;
-using chromeos::NetworkState;
+using chromeos::network_config::mojom::NetworkStateProperties;
+using chromeos::network_config::mojom::NetworkType;
 
 namespace ash {
 
@@ -24,32 +23,23 @@ namespace {
 
 // Returns true if the network is actually toggled.
 bool SetNetworkEnabled(bool enabled) {
-  const NetworkState* network =
-      NetworkHandler::Get()->network_state_handler()->ConnectedNetworkByType(
-          NetworkTypePattern::NonVirtual());
+  TrayNetworkStateModel* model =
+      Shell::Get()->system_tray_model()->network_state_model();
+  const NetworkStateProperties* network = model->default_network();
 
   // For cellular and tether, users are only allowed to disable them from
   // feature pod toggle.
-  if (!enabled && network && network->Matches(NetworkTypePattern::Cellular())) {
-    NetworkHandler::Get()->network_state_handler()->SetTechnologyEnabled(
-        NetworkTypePattern::Cellular(), false,
-        chromeos::network_handler::ErrorCallback());
+  if (!enabled && network &&
+      (network->type == NetworkType::kCellular ||
+       network->type == NetworkType::kTether)) {
+    model->SetNetworkTypeEnabledState(network->type, false);
     return true;
   }
 
-  if (!enabled && network && network->Matches(NetworkTypePattern::Tether())) {
-    NetworkHandler::Get()->network_state_handler()->SetTechnologyEnabled(
-        NetworkTypePattern::Tether(), false,
-        chromeos::network_handler::ErrorCallback());
-    return true;
-  }
-
-  if (network && !network->Matches(NetworkTypePattern::WiFi()))
+  if (network && network->type != NetworkType::kWiFi)
     return false;
 
-  NetworkHandler::Get()->network_state_handler()->SetTechnologyEnabled(
-      NetworkTypePattern::WiFi(), enabled,
-      chromeos::network_handler::ErrorCallback());
+  model->SetNetworkTypeEnabledState(NetworkType::kWiFi, enabled);
   return true;
 }
 

ash/system/network/tray_network_state_model.cc

@@ -68,7 +68,8 @@ void TrayNetworkStateModel::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
-DeviceStateProperties* TrayNetworkStateModel::GetDevice(NetworkType type) {
+const DeviceStateProperties* TrayNetworkStateModel::GetDevice(
+    NetworkType type) const {
   auto iter = devices_.find(type);
   if (iter == devices_.end())
     return nullptr;
@@ -76,10 +77,17 @@ DeviceStateProperties* TrayNetworkStateModel::GetDevice(NetworkType type) {
 }
 
 DeviceStateType TrayNetworkStateModel::GetDeviceState(NetworkType type) {
-  DeviceStateProperties* device = GetDevice(type);
+  const DeviceStateProperties* device = GetDevice(type);
   return device ? device->state : DeviceStateType::kUnavailable;
 }
 
+void TrayNetworkStateModel::SetNetworkTypeEnabledState(NetworkType type,
+                                                       bool enabled) {
+  DCHECK(cros_network_config_ptr_);
+  cros_network_config_ptr_->SetNetworkTypeEnabledState(type, enabled,
+                                                       base::DoNothing());
+}
+
 // CrosNetworkConfigObserver
 
 void TrayNetworkStateModel::OnActiveNetworksChanged(
@@ -116,6 +124,7 @@ void TrayNetworkStateModel::BindCrosNetworkConfig(
 }
 
 void TrayNetworkStateModel::GetDeviceStateList() {
+  DCHECK(cros_network_config_ptr_);
   cros_network_config_ptr_->GetDeviceStateList(base::BindOnce(
       &TrayNetworkStateModel::OnGetDeviceStateList, base::Unretained(this)));
 }

ash/system/network/tray_network_state_model.h

@@ -43,24 +43,32 @@ class ASH_EXPORT TrayNetworkStateModel
   void RemoveObserver(Observer* observer);
 
   // Returns DeviceStateProperties for |type| if it exists or null.
-  chromeos::network_config::mojom::DeviceStateProperties* GetDevice(
-      chromeos::network_config::mojom::NetworkType type);
+  const chromeos::network_config::mojom::DeviceStateProperties* GetDevice(
+      chromeos::network_config::mojom::NetworkType type) const;
 
   // Returns the DeviceStateType for |type| if a device exists or kUnavailable.
   chromeos::network_config::mojom::DeviceStateType GetDeviceState(
       chromeos::network_config::mojom::NetworkType type);
 
-  chromeos::network_config::mojom::NetworkStateProperties* default_network() {
+  // Convenience method to call cros_network_config_ptr_ method.
+  void SetNetworkTypeEnabledState(
+      chromeos::network_config::mojom::NetworkType type,
+      bool enabled);
+
+  const chromeos::network_config::mojom::NetworkStateProperties*
+  default_network() const {
     return default_network_.get();
   }
-  chromeos::network_config::mojom::NetworkStateProperties*
-  active_non_cellular() {
+  const chromeos::network_config::mojom::NetworkStateProperties*
+  active_non_cellular() const {
     return active_non_cellular_.get();
   }
-  chromeos::network_config::mojom::NetworkStateProperties* active_cellular() {
+  const chromeos::network_config::mojom::NetworkStateProperties*
+  active_cellular() const {
     return active_cellular_.get();
   }
-  chromeos::network_config::mojom::NetworkStateProperties* active_vpn() {
+  const chromeos::network_config::mojom::NetworkStateProperties* active_vpn()
+      const {
     return active_vpn_.get();
   }
 

ash/system/network/vpn_feature_pod_controller.cc

@@ -14,15 +14,11 @@
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/unified/feature_pod_button.h"
 #include "ash/system/unified/unified_system_tray_controller.h"
-#include "chromeos/network/network_state.h"
-#include "chromeos/network/network_state_handler.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
 
-using chromeos::NetworkHandler;
-using chromeos::NetworkState;
-using chromeos::NetworkStateHandler;
-using chromeos::NetworkTypePattern;
+using chromeos::network_config::mojom::ConnectionStateType;
+using chromeos::network_config::mojom::NetworkStateProperties;
 
 namespace ash {
 
@@ -39,24 +35,7 @@ bool IsVPNVisibleInSystemTray() {
     return true;
 
   // Also show the VPN entry if at least one VPN network is configured.
-  NetworkStateHandler* const handler =
-      NetworkHandler::Get()->network_state_handler();
-  if (handler->FirstNetworkByType(NetworkTypePattern::VPN()))
-    return true;
-  return false;
-}
-
-bool IsVPNEnabled() {
-  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
-  return handler->FirstNetworkByType(NetworkTypePattern::VPN());
-}
-
-bool IsVPNConnected() {
-  NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
-  const NetworkState* vpn =
-      handler->FirstNetworkByType(NetworkTypePattern::VPN());
-  return IsVPNEnabled() &&
-         (vpn->IsConnectedState() || vpn->IsConnectingState());
+  return Shell::Get()->system_tray_model()->network_state_model()->active_vpn();
 }
 
 }  // namespace
@@ -98,18 +77,18 @@ void VPNFeaturePodController::ActiveNetworkStateChanged() {
 }
 
 void VPNFeaturePodController::Update() {
-  // NetworkHandler can be uninitialized in unit tests.
-  if (!chromeos::NetworkHandler::IsInitialized())
-    return;
-
   button_->SetVisible(IsVPNVisibleInSystemTray());
   if (!button_->GetVisible())
     return;
 
+  const NetworkStateProperties* vpn =
+      Shell::Get()->system_tray_model()->network_state_model()->active_vpn();
+  bool is_active =
+      vpn && vpn->connection_state != ConnectionStateType::kNotConnected;
   button_->SetSubLabel(l10n_util::GetStringUTF16(
-      IsVPNConnected() ? IDS_ASH_STATUS_TRAY_VPN_CONNECTED_SHORT
-                       : IDS_ASH_STATUS_TRAY_VPN_DISCONNECTED_SHORT));
-  button_->SetToggled(IsVPNEnabled() && IsVPNConnected());
+      is_active ? IDS_ASH_STATUS_TRAY_VPN_CONNECTED_SHORT
+                : IDS_ASH_STATUS_TRAY_VPN_DISCONNECTED_SHORT));
+  button_->SetToggled(is_active);
 }
 
 }  // namespace ash

chromeos/services/network_config/cros_network_config.cc

@@ -65,8 +65,8 @@ NetworkTypePattern MojoTypeToPattern(mojom::NetworkType type) {
       return NetworkTypePattern::WiFi();
     case mojom::NetworkType::kWiMAX:
       return NetworkTypePattern::Wimax();
-      NOTREACHED();
   }
+  NOTREACHED();
   return NetworkTypePattern::Default();
 }
 
@@ -252,6 +252,24 @@ mojom::DeviceStatePropertiesPtr DeviceStateToMojo(
   return result;
 }
 
+bool NetworkTypeCanBeDisabled(mojom::NetworkType type) {
+  switch (type) {
+    case mojom::NetworkType::kCellular:
+    case mojom::NetworkType::kTether:
+    case mojom::NetworkType::kWiFi:
+    case mojom::NetworkType::kWiMAX:
+      return true;
+    case mojom::NetworkType::kAll:
+    case mojom::NetworkType::kEthernet:
+    case mojom::NetworkType::kMobile:
+    case mojom::NetworkType::kVPN:
+    case mojom::NetworkType::kWireless:
+      return false;
+  }
+  NOTREACHED();
+  return false;
+}
+
 }  // namespace
 
 CrosNetworkConfig::CrosNetworkConfig(NetworkStateHandler* network_state_handler)
@@ -347,6 +365,32 @@ void CrosNetworkConfig::GetDeviceStateList(
   std::move(callback).Run(std::move(result));
 }
 
+void CrosNetworkConfig::SetNetworkTypeEnabledState(
+    mojom::NetworkType type,
+    bool enabled,
+    SetNetworkTypeEnabledStateCallback callback) {
+  if (!NetworkTypeCanBeDisabled(type)) {
+    std::move(callback).Run(false);
+    return;
+  }
+  NetworkTypePattern pattern = MojoTypeToPattern(type);
+  if (!network_state_handler_->IsTechnologyAvailable(pattern)) {
+    NET_LOG(ERROR) << "Technology unavailable: " << type;
+    std::move(callback).Run(false);
+    return;
+  }
+  if (network_state_handler_->IsTechnologyProhibited(pattern)) {
+    NET_LOG(ERROR) << "Technology prohibited: " << type;
+    std::move(callback).Run(false);
+    return;
+  }
+  // Set the technology enabled state and return true. The call to Shill does
+  // not have a 'success' callback (and errors are already logged).
+  network_state_handler_->SetTechnologyEnabled(
+      pattern, enabled, chromeos::network_handler::ErrorCallback());
+  std::move(callback).Run(true);
+}
+
 // NetworkStateHandlerObserver
 void CrosNetworkConfig::NetworkListChanged() {
   observers_.ForAllPtrs([](mojom::CrosNetworkConfigObserver* observer) {

chromeos/services/network_config/cros_network_config.h

@@ -31,6 +31,10 @@ class CrosNetworkConfig : public mojom::CrosNetworkConfig,
   void GetNetworkStateList(mojom::NetworkFilterPtr filter,
                            GetNetworkStateListCallback callback) override;
   void GetDeviceStateList(GetDeviceStateListCallback callback) override;
+  void SetNetworkTypeEnabledState(
+      mojom::NetworkType type,
+      bool enabled,
+      SetNetworkTypeEnabledStateCallback callback) override;
 
   // NetworkStateHandlerObserver
   void NetworkListChanged() override;

chromeos/services/network_config/cros_network_config_unittest.cc

@@ -249,6 +249,31 @@ TEST_F(CrosNetworkConfigTest, GetDeviceStateList) {
       }));
 }
 
+TEST_F(CrosNetworkConfigTest, SetNetworkTypeEnabledState) {
+  cros_network_config()->GetDeviceStateList(
+      base::BindOnce([](std::vector<mojom::DeviceStatePropertiesPtr> devices) {
+        ASSERT_EQ(3u, devices.size());
+        EXPECT_EQ(mojom::NetworkType::kWiFi, devices[0]->type);
+        EXPECT_EQ(mojom::DeviceStateType::kEnabled, devices[0]->state);
+      }));
+  // Disable WiFi
+  bool succeeded = false;
+  cros_network_config()->SetNetworkTypeEnabledState(
+      mojom::NetworkType::kWiFi, false,
+      base::BindOnce(
+          [](bool* succeeded, bool success) { *succeeded = success; },
+          &succeeded));
+  // Wait for callback to complete; this test does not use mojo bindings.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(succeeded);
+  cros_network_config()->GetDeviceStateList(
+      base::BindOnce([](std::vector<mojom::DeviceStatePropertiesPtr> devices) {
+        ASSERT_EQ(3u, devices.size());
+        EXPECT_EQ(mojom::NetworkType::kWiFi, devices[0]->type);
+        EXPECT_EQ(mojom::DeviceStateType::kDisabled, devices[0]->state);
+      }));
+}
+
 TEST_F(CrosNetworkConfigTest, NetworkListChanged) {
   SetupObserver();
   base::RunLoop().RunUntilIdle();

chromeos/services/network_config/public/mojom/cros_network_config.mojom

@@ -239,6 +239,13 @@ interface CrosNetworkConfig {
 
   // Returns a list of Device properties.
   GetDeviceStateList() => (array<DeviceStateProperties> result);
+
+  // Sets a single network type (e.g. WiFi) to enabled or disabled. Types
+  // describing multiple technologies (e.g. kWireless) are not supported.
+  // Returns false if the type is invalid, unavailable, or can not be disabled.
+  // CrosNetworkConfigObserver::OnDeviceStateListChanged() can be used to be
+  // notified when the enabled state changes.
+  SetNetworkTypeEnabledState(NetworkType type, bool enabled) => (bool success);
 };
 
 interface CrosNetworkConfigObserver {
@@ -247,8 +254,12 @@ interface CrosNetworkConfigObserver {
   OnActiveNetworksChanged(array<NetworkStateProperties> networks);
 
   // Fired when the list of networks changes.
+  // CrosNetworkConfig::GetNetworkStateList() can be used to get the network
+  // states.
   OnNetworkStateListChanged();
 
   // Fired when the list of devices changes or a device property changes.
+  // CrosNetworkConfig::GetDeviceStateList() can be used to retrieve the device
+  // states.
   OnDeviceStateListChanged();
 };