NetworkDeviceHandler: Use single callback for GetDeviceProperties

This is a part of a greater effort to cleanup callbacks in
chromeos/network.

GetDeviceProperties causes complications in Get*Properties calls,
which will be cleaned up in followup CLs.

Bug: 1007660
Change-Id: I4039ea1a672890b6c24cb6057394377c302c6200
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2291233
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787934}

chrome/browser/ui/webui/chromeos/network_ui.cc

@@ -198,12 +198,8 @@ class NetworkConfigMessageHandler : public content::WebUIMessageHandler {
     }
     NetworkHandler::Get()->network_device_handler()->GetDeviceProperties(
         device->path(),
-        base::BindOnce(
-            &NetworkConfigMessageHandler::GetShillDevicePropertiesSuccess,
-            weak_ptr_factory_.GetWeakPtr(), callback_id),
-        base::Bind(&NetworkConfigMessageHandler::ErrorCallback,
-                   weak_ptr_factory_.GetWeakPtr(), callback_id, type,
-                   kGetDeviceProperties));
+        base::BindOnce(&NetworkConfigMessageHandler::OnGetShillDeviceProperties,
+                       weak_ptr_factory_.GetWeakPtr(), callback_id, type));
   }
 
   void GetShillEthernetEAP(const base::ListValue* arg_list) {
@@ -271,18 +267,21 @@ class NetworkConfigMessageHandler : public content::WebUIMessageHandler {
     InternetConfigDialog::ShowDialogForNetworkType(::onc::network_type::kWiFi);
   }
 
-  void GetShillDevicePropertiesSuccess(
-      const std::string& callback_id,
-      const std::string& device_path,
-      const base::DictionaryValue& dictionary) {
-    std::unique_ptr<base::DictionaryValue> dictionary_copy(
-        dictionary.DeepCopy());
+  void OnGetShillDeviceProperties(const std::string& callback_id,
+                                  const std::string& type,
+                                  const std::string& device_path,
+                                  base::Optional<base::Value> result) {
+    if (!result) {
+      ErrorCallback(callback_id, type, kGetDeviceProperties,
+                    "GetDeviceProperties failed", nullptr);
+      return;
+    }
 
     // Set the 'device_path' property for debugging.
-    dictionary_copy->SetKey("device_path", base::Value(device_path));
+    result->SetKey("device_path", base::Value(device_path));
 
     base::ListValue return_arg_list;
-    return_arg_list.Append(std::move(dictionary_copy));
+    return_arg_list.Append(std::move(*result));
     Respond(callback_id, return_arg_list);
   }
 

chromeos/network/fake_network_device_handler.cc

@@ -12,8 +12,7 @@ FakeNetworkDeviceHandler::~FakeNetworkDeviceHandler() = default;
 
 void FakeNetworkDeviceHandler::GetDeviceProperties(
     const std::string& device_path,
-    network_handler::DictionaryResultCallback callback,
-    const network_handler::ErrorCallback& error_callback) const {}
+    network_handler::ResultCallback callback) const {}
 
 void FakeNetworkDeviceHandler::SetDeviceProperty(
     const std::string& device_path,

chromeos/network/fake_network_device_handler.h

@@ -26,8 +26,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) FakeNetworkDeviceHandler
   // NetworkDeviceHandler overrides
   void GetDeviceProperties(
       const std::string& device_path,
-      network_handler::DictionaryResultCallback callback,
-      const network_handler::ErrorCallback& error_callback) const override;
+      network_handler::ResultCallback callback) const override;
 
   void SetDeviceProperty(
       const std::string& device_path,

chromeos/network/managed_network_configuration_handler_impl.cc

@@ -1100,42 +1100,29 @@ void ManagedNetworkConfigurationHandlerImpl::GetPropertiesCallback(
 
   // Request the device properties. On success or failure pass (a possibly
   // modified) |shill_properties| to |send_callback|.
-  std::unique_ptr<base::DictionaryValue> shill_properties_copy_error_copy(
-      shill_properties_copy->DeepCopy());
-  auto repeating_send_callback =
-      base::AdaptCallbackForRepeating(std::move(send_callback));
   network_device_handler_->GetDeviceProperties(
       device_path,
       base::BindOnce(
-          &ManagedNetworkConfigurationHandlerImpl::GetDevicePropertiesSuccess,
+          &ManagedNetworkConfigurationHandlerImpl::OnGetDeviceProperties,
           weak_ptr_factory_.GetWeakPtr(), service_path,
-          std::move(shill_properties_copy), repeating_send_callback),
-      base::Bind(
-          &ManagedNetworkConfigurationHandlerImpl::GetDevicePropertiesFailure,
-          weak_ptr_factory_.GetWeakPtr(), service_path,
-          base::Passed(&shill_properties_copy_error_copy),
-          repeating_send_callback));
+          std::move(shill_properties_copy), std::move(send_callback)));
 }
 
-void ManagedNetworkConfigurationHandlerImpl::GetDevicePropertiesSuccess(
+void ManagedNetworkConfigurationHandlerImpl::OnGetDeviceProperties(
     const std::string& service_path,
     std::unique_ptr<base::DictionaryValue> network_properties,
     GetDevicePropertiesCallback send_callback,
     const std::string& device_path,
-    const base::DictionaryValue& device_properties) {
+    base::Optional<base::Value> device_properties) {
+  if (!device_properties) {
+    NET_LOG(ERROR) << "Error getting device properties: "
+                   << NetworkPathId(service_path);
+    std::move(send_callback).Run(service_path, std::move(network_properties));
+    return;
+  }
   // Create a "Device" dictionary in |network_properties|.
-  network_properties->SetKey(shill::kDeviceProperty, device_properties.Clone());
-  std::move(send_callback).Run(service_path, std::move(network_properties));
-}
-
-void ManagedNetworkConfigurationHandlerImpl::GetDevicePropertiesFailure(
-    const std::string& service_path,
-    std::unique_ptr<base::DictionaryValue> network_properties,
-    GetDevicePropertiesCallback send_callback,
-    const std::string& error_name,
-    std::unique_ptr<base::DictionaryValue> error_data) {
-  NET_LOG(ERROR) << "Error getting device properties: "
-                 << NetworkPathId(service_path);
+  network_properties->SetKey(shill::kDeviceProperty,
+                             std::move(*device_properties));
   std::move(send_callback).Run(service_path, std::move(network_properties));
 }
 

chromeos/network/managed_network_configuration_handler_impl.h

@@ -200,18 +200,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ManagedNetworkConfigurationHandlerImpl
       const std::string& service_path,
       const base::DictionaryValue& shill_properties);
 
-  void GetDevicePropertiesSuccess(
+  void OnGetDeviceProperties(
       const std::string& service_path,
       std::unique_ptr<base::DictionaryValue> network_properties,
       GetDevicePropertiesCallback send_callback,
       const std::string& device_path,
-      const base::DictionaryValue& device_properties);
-  void GetDevicePropertiesFailure(
-      const std::string& service_path,
-      std::unique_ptr<base::DictionaryValue> network_properties,
-      GetDevicePropertiesCallback send_callback,
-      const std::string& error_name,
-      std::unique_ptr<base::DictionaryValue> error_data);
+      base::Optional<base::Value> device_properties);
 
   // Called from SetProperties, calls NCH::SetShillProperties.
   void SetShillProperties(

chromeos/network/mock_network_device_handler.h

@@ -25,11 +25,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) MockNetworkDeviceHandler
   MockNetworkDeviceHandler();
   virtual ~MockNetworkDeviceHandler();
 
-  MOCK_CONST_METHOD3(
-      GetDeviceProperties,
-      void(const std::string& device_path,
-           network_handler::DictionaryResultCallback callback,
-           const network_handler::ErrorCallback& error_callback));
+  MOCK_CONST_METHOD2(GetDeviceProperties,
+                     void(const std::string& device_path,
+                          network_handler::ResultCallback callback));
 
   MOCK_METHOD5(SetDeviceProperty,
                void(const std::string& device_path,

chromeos/network/network_device_handler.h

@@ -54,12 +54,11 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkDeviceHandler {
   NetworkDeviceHandler();
   virtual ~NetworkDeviceHandler();
 
-  // Gets the properties of the device with id |device_path|. See note on
-  // |callback| and |error_callback|, in class description above.
+  // Invokes |callback| with the properties for the device matching
+  // |device_path| on success, or nullopt on failure.
   virtual void GetDeviceProperties(
       const std::string& device_path,
-      network_handler::DictionaryResultCallback callback,
-      const network_handler::ErrorCallback& error_callback) const = 0;
+      network_handler::ResultCallback callback) const = 0;
 
   // Sets the value of property |name| on device with id |device_path| to
   // |value|. This function provides a generic setter to be used by the UI or

chromeos/network/network_device_handler_impl.cc

@@ -25,8 +25,8 @@
 #include "chromeos/dbus/shill/shill_device_client.h"
 #include "chromeos/dbus/shill/shill_ipconfig_client.h"
 #include "chromeos/network/device_state.h"
+#include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state_handler.h"
-#include "components/device_event_log/device_event_log.h"
 #include "dbus/object_path.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -52,6 +52,18 @@ std::string GetErrorNameForShillError(const std::string& shill_error_name) {
   return NetworkDeviceHandler::kErrorUnknown;
 }
 
+void GetPropertiesCallback(const std::string& device_path,
+                           network_handler::ResultCallback callback,
+                           DBusMethodCallStatus call_status,
+                           base::Value result) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    NET_LOG(ERROR) << "GetProperties failed: " << NetworkPathId(device_path)
+                   << " Status: " << call_status;
+    std::move(callback).Run(device_path, base::nullopt);
+  }
+  std::move(callback).Run(device_path, std::move(result));
+}
+
 void InvokeErrorCallback(const std::string& device_path,
                          const network_handler::ErrorCallback& error_callback,
                          const std::string& error_name) {
@@ -230,12 +242,10 @@ NetworkDeviceHandlerImpl::~NetworkDeviceHandlerImpl() {
 
 void NetworkDeviceHandlerImpl::GetDeviceProperties(
     const std::string& device_path,
-    network_handler::DictionaryResultCallback callback,
-    const network_handler::ErrorCallback& error_callback) const {
+    network_handler::ResultCallback callback) const {
   ShillDeviceClient::Get()->GetProperties(
       dbus::ObjectPath(device_path),
-      base::BindOnce(&network_handler::GetPropertiesCallback,
-                     std::move(callback), error_callback, device_path));
+      base::BindOnce(&GetPropertiesCallback, device_path, std::move(callback)));
 }
 
 void NetworkDeviceHandlerImpl::SetDeviceProperty(
@@ -517,8 +527,7 @@ void NetworkDeviceHandlerImpl::ApplyMACAddressRandomizationToShill() {
           device_state->path(),
           base::BindOnce(
               &NetworkDeviceHandlerImpl::HandleMACAddressRandomization,
-              weak_ptr_factory_.GetWeakPtr()),
-          network_handler::ErrorCallback());
+              weak_ptr_factory_.GetWeakPtr()));
       return;
     case MACAddressRandomizationSupport::SUPPORTED:
       SetDevicePropertyInternal(
@@ -665,10 +674,12 @@ void NetworkDeviceHandlerImpl::
 
 void NetworkDeviceHandlerImpl::HandleMACAddressRandomization(
     const std::string& device_path,
-    const base::DictionaryValue& properties) {
-  bool supported;
-  if (!properties.GetBooleanWithoutPathExpansion(
-          shill::kMacAddressRandomizationSupportedProperty, &supported)) {
+    base::Optional<base::Value> properties) {
+  if (!properties)
+    return;
+  base::Optional<bool> supported =
+      properties->FindBoolKey(shill::kMacAddressRandomizationSupportedProperty);
+  if (!supported.has_value()) {
     if (base::SysInfo::IsRunningOnChromeOS()) {
       NET_LOG(ERROR) << "Failed to determine if device " << device_path
                      << " supports MAC address randomization";
@@ -677,7 +688,7 @@ void NetworkDeviceHandlerImpl::HandleMACAddressRandomization(
   }
 
   // Try to set MAC address randomization if it's supported.
-  if (supported) {
+  if (*supported) {
     mac_addr_randomization_supported_ =
         MACAddressRandomizationSupport::SUPPORTED;
     ApplyMACAddressRandomizationToShill();

chromeos/network/network_device_handler_impl.h

@@ -32,8 +32,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkDeviceHandlerImpl
   // NetworkDeviceHandler overrides
   void GetDeviceProperties(
       const std::string& device_path,
-      network_handler::DictionaryResultCallback callback,
-      const network_handler::ErrorCallback& error_callback) const override;
+      network_handler::ResultCallback callback) const override;
 
   void SetDeviceProperty(
       const std::string& device_path,
@@ -178,7 +177,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkDeviceHandlerImpl
   // supported, also apply |mac_addr_randomization_enabled_| to the
   // shill device.
   void HandleMACAddressRandomization(const std::string& device_path,
-                                     const base::DictionaryValue& properties);
+                                     base::Optional<base::Value> properties);
 
   // Get the DeviceState for the wifi device, if any.
   const DeviceState* GetWifiDeviceState(

chromeos/network/network_device_handler_unittest.cc

@@ -27,6 +27,7 @@ namespace {
 const char kDefaultCellularDevicePath[] = "stub_cellular_device";
 const char kUnknownCellularDevicePath[] = "unknown_cellular_device";
 const char kDefaultWifiDevicePath[] = "stub_wifi_device";
+const char kResultFailure[] = "failure";
 const char kResultSuccess[] = "success";
 const char kDefaultPin[] = "1111";
 
@@ -88,10 +89,15 @@ class NetworkDeviceHandlerTest : public testing::Test {
 
   void SuccessCallback() { result_ = kResultSuccess; }
 
-  void PropertiesSuccessCallback(const std::string& device_path,
-                                 const base::DictionaryValue& properties) {
+  void GetPropertiesCallback(const std::string& device_path,
+                             base::Optional<base::Value> properties) {
+    if (!properties) {
+      result_ = kResultFailure;
+      return;
+    }
     result_ = kResultSuccess;
-    properties_.reset(properties.DeepCopy());
+    properties_ = base::DictionaryValue::From(
+        std::make_unique<base::Value>(std::move(*properties)));
   }
 
   void StringSuccessCallback(const std::string& result) {
@@ -103,9 +109,8 @@ class NetworkDeviceHandlerTest : public testing::Test {
                            const std::string& expected_result) {
     network_device_handler_->GetDeviceProperties(
         device_path,
-        base::BindOnce(&NetworkDeviceHandlerTest::PropertiesSuccessCallback,
-                       base::Unretained(this)),
-        error_callback_);
+        base::BindOnce(&NetworkDeviceHandlerTest::GetPropertiesCallback,
+                       base::Unretained(this)));
     base::RunLoop().RunUntilIdle();
     ASSERT_EQ(expected_result, result_);
   }

chromeos/network/network_handler_callbacks.h

@@ -23,6 +23,13 @@ COMPONENT_EXPORT(CHROMEOS_NETWORK) extern const char kErrorDetail[];
 COMPONENT_EXPORT(CHROMEOS_NETWORK) extern const char kDbusErrorName[];
 COMPONENT_EXPORT(CHROMEOS_NETWORK) extern const char kDbusErrorMessage[];
 
+// Modern callback for updated methods using a single OnceCallback with an
+// optional result. This can be used when error logging is handled at the point
+// of failure and there is no need to pass additional details to the caller,
+// other than a nullopt to indicate failure.
+using ResultCallback = base::OnceCallback<void(const std::string& service_path,
+                                               base::Optional<base::Value>)>;
+
 // An error callback used by both the configuration handler and the state
 // handler to receive error results from the API.
 typedef base::RepeatingCallback<void(