Migrate ModemMessagingClient into DBusMethodCallback.

BUG=739622
TEST=Ran chromeos_unittests.

Change-Id: Id389c09d95edae181f7862174ab58de765b0ee22
Reviewed-on: https://chromium-review.googlesource.com/937741
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541295}

chromeos/dbus/fake_modem_messaging_client.cc

@@ -35,29 +35,28 @@ void FakeModemMessagingClient::ResetSmsReceivedHandler(
 void FakeModemMessagingClient::Delete(const std::string& service_name,
                                       const dbus::ObjectPath& object_path,
                                       const dbus::ObjectPath& sms_path,
-                                      const DeleteCallback& callback) {
+                                      VoidDBusMethodCallback callback) {
   std::vector<dbus::ObjectPath>::iterator it(
       find(message_paths_.begin(), message_paths_.end(), sms_path));
   if (it != message_paths_.end())
     message_paths_.erase(it);
-  callback.Run();
+  std::move(callback).Run(true);
 }
 
 void FakeModemMessagingClient::List(const std::string& service_name,
                                     const dbus::ObjectPath& object_path,
-                                    const ListCallback& callback) {
+                                    ListCallback callback) {
   // This entire FakeModemMessagingClient is for testing.
   // Calling List with |service_name| equal to "AddSMS" allows unit
   // tests to confirm that the sms_received_handler is functioning.
   if (service_name == "AddSMS") {
-    std::vector<dbus::ObjectPath> no_paths;
     const dbus::ObjectPath kSmsPath("/SMS/0");
     message_paths_.push_back(kSmsPath);
     if (!sms_received_handler_.is_null())
       sms_received_handler_.Run(kSmsPath, true);
-    callback.Run(no_paths);
+    std::move(callback).Run({});
   } else {
-    callback.Run(message_paths_);
+    std::move(callback).Run(message_paths_);
   }
 }
 

chromeos/dbus/fake_modem_messaging_client.h

@@ -29,10 +29,10 @@ class CHROMEOS_EXPORT FakeModemMessagingClient : public ModemMessagingClient {
   void Delete(const std::string& service_name,
               const dbus::ObjectPath& object_path,
               const dbus::ObjectPath& sms_path,
-              const DeleteCallback& callback) override;
+              VoidDBusMethodCallback callback) override;
   void List(const std::string& service_name,
             const dbus::ObjectPath& object_path,
-            const ListCallback& callback) override;
+            ListCallback callback) override;
 
  private:
   SmsReceivedHandler sms_received_handler_;

chromeos/dbus/modem_messaging_client.cc

@@ -26,8 +26,7 @@ namespace {
 class ModemMessagingProxy {
  public:
   typedef ModemMessagingClient::SmsReceivedHandler SmsReceivedHandler;
-  typedef ModemMessagingClient::ListCallback ListCallback;
-  typedef ModemMessagingClient::DeleteCallback DeleteCallback;
+  using ListCallback = ModemMessagingClient::ListCallback;
 
   ModemMessagingProxy(dbus::Bus* bus,
            const std::string& service_name,
@@ -58,7 +57,7 @@ class ModemMessagingProxy {
 
   // Calls Delete method.
   void Delete(const dbus::ObjectPath& message_path,
-              const DeleteCallback& callback) {
+              VoidDBusMethodCallback callback) {
     dbus::MethodCall method_call(modemmanager::kModemManager1MessagingInterface,
                                  modemmanager::kSMSDeleteFunction);
     dbus::MessageWriter writer(&method_call);
@@ -66,17 +65,17 @@ class ModemMessagingProxy {
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&ModemMessagingProxy::OnDelete,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   // Calls List method.
-  virtual void List(const ListCallback& callback) {
+  virtual void List(ListCallback callback) {
     dbus::MethodCall method_call(modemmanager::kModemManager1MessagingInterface,
                                  modemmanager::kSMSListFunction);
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&ModemMessagingProxy::OnList,
-                       weak_ptr_factory_.GetWeakPtr(), callback));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
  private:
@@ -96,21 +95,24 @@ class ModemMessagingProxy {
   }
 
   // Handles responses of Delete method calls.
-  void OnDelete(const DeleteCallback& callback, dbus::Response* response) {
-    if (!response)
-      return;
-    callback.Run();
+  void OnDelete(VoidDBusMethodCallback callback, dbus::Response* response) {
+    std::move(callback).Run(response);
   }
 
   // Handles responses of List method calls.
-  void OnList(const ListCallback& callback, dbus::Response* response) {
-    if (!response)
+  void OnList(ListCallback callback, dbus::Response* response) {
+    if (!response) {
+      std::move(callback).Run(base::nullopt);
       return;
+    }
     dbus::MessageReader reader(response);
     std::vector<dbus::ObjectPath> sms_paths;
-    if (!reader.PopArrayOfObjectPaths(&sms_paths))
+    if (!reader.PopArrayOfObjectPaths(&sms_paths)) {
       LOG(WARNING) << "Invalid response: " << response->ToString();
-    callback.Run(sms_paths);
+      std::move(callback).Run(base::nullopt);
+      return;
+    }
+    std::move(callback).Run(std::move(sms_paths));
   }
 
   // Handles the result of signal connection setup.
@@ -150,14 +152,14 @@ class CHROMEOS_EXPORT ModemMessagingClientImpl : public ModemMessagingClient {
   void Delete(const std::string& service_name,
               const dbus::ObjectPath& object_path,
               const dbus::ObjectPath& sms_path,
-              const DeleteCallback& callback) override {
-    GetProxy(service_name, object_path)->Delete(sms_path, callback);
+              VoidDBusMethodCallback callback) override {
+    GetProxy(service_name, object_path)->Delete(sms_path, std::move(callback));
   }
 
   void List(const std::string& service_name,
             const dbus::ObjectPath& object_path,
-            const ListCallback& callback) override {
-    GetProxy(service_name, object_path)->List(callback);
+            ListCallback callback) override {
+    GetProxy(service_name, object_path)->List(std::move(callback));
   }
 
  protected:

chromeos/dbus/modem_messaging_client.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_client.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
 
 namespace dbus {
 class ObjectPath;
@@ -25,11 +26,8 @@ namespace chromeos {
 // initializes the DBusThreadManager instance.
 class CHROMEOS_EXPORT ModemMessagingClient : public DBusClient {
  public:
-  typedef base::Callback<void()> DeleteCallback;
   typedef base::Callback<void(const dbus::ObjectPath& message_path,
                               bool complete)> SmsReceivedHandler;
-  typedef base::Callback<void(const std::vector<dbus::ObjectPath>& paths)>
-      ListCallback;
 
   ~ModemMessagingClient() override;
 
@@ -46,16 +44,17 @@ class CHROMEOS_EXPORT ModemMessagingClient : public DBusClient {
   virtual void ResetSmsReceivedHandler(const std::string& service_name,
                                        const dbus::ObjectPath& object_path) = 0;
 
-  // Calls Delete method.  |callback| is called after the method call succeeds.
+  // Calls Delete method.  |callback| is called on method completion.
   virtual void Delete(const std::string& service_name,
                       const dbus::ObjectPath& object_path,
                       const dbus::ObjectPath& sms_path,
-                      const DeleteCallback& callback) = 0;
+                      VoidDBusMethodCallback callback) = 0;
 
-  // Calls List method.  |callback| is called after the method call succeeds.
+  // Calls List method.  |callback| is called on method completion.
+  using ListCallback = DBusMethodCallback<std::vector<dbus::ObjectPath>>;
   virtual void List(const std::string& service_name,
                     const dbus::ObjectPath& object_path,
-                    const ListCallback& callback) = 0;
+                    ListCallback callback) = 0;
 
  protected:
   friend class ModemMessagingClientTest;

chromeos/dbus/modem_messaging_client_unittest.cc

@@ -185,13 +185,15 @@ TEST_F(ModemMessagingClientTest, Delete) {
   std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
   response_ = response.get();
   // Call Delete.
-  bool called = false;
+  bool success = false;
   client_->Delete(kServiceName, dbus::ObjectPath(kObjectPath), kSmsPath,
-                  base::Bind([](bool* called) { *called = true; }, &called));
+                  base::BindOnce([](bool* success_out,
+                                    bool success) { *success_out = true; },
+                                 &success));
 
   // Run the message loop.
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(called);
+  EXPECT_TRUE(success);
 }
 
 TEST_F(ModemMessagingClientTest, List) {
@@ -214,18 +216,14 @@ TEST_F(ModemMessagingClientTest, List) {
       kServiceName, dbus::ObjectPath(kObjectPath),
       base::Bind(
           [](base::Optional<std::vector<dbus::ObjectPath>>* result_out,
-             const std::vector<dbus::ObjectPath>& result) {
-            result_out->emplace(result);
+             base::Optional<std::vector<dbus::ObjectPath>> result) {
+            *result_out = std::move(result);
           },
           &result));
 
   // Run the message loop.
   base::RunLoop().RunUntilIdle();
-
-  // TODO(crbug.com/784732): We should be able to skip explicit has_value()
-  // check.
-  ASSERT_TRUE(result.has_value());
-  EXPECT_EQ(kExpectedResult, result.value());
+  EXPECT_EQ(kExpectedResult, result);
 }
 
 }  // namespace chromeos

chromeos/network/network_sms_handler.cc

@@ -193,10 +193,11 @@ class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
   void RequestUpdate() override;
 
  private:
-  void ListCallback(const std::vector<dbus::ObjectPath>& paths);
+  void ListCallback(base::Optional<std::vector<dbus::ObjectPath>> paths);
   void SmsReceivedCallback(const dbus::ObjectPath& path, bool complete);
   void GetCallback(const base::DictionaryValue& dictionary);
   void DeleteMessages();
+  void DeleteCallback(bool success);
   void GetMessages();
   void MessageReceived(const base::DictionaryValue& dictionary);
 
@@ -234,9 +235,9 @@ ModemManager1NetworkSmsDeviceHandler::ModemManager1NetworkSmsDeviceHandler(
   // List the existing messages.
   DBusThreadManager::Get()->GetModemMessagingClient()->List(
       service_name_, object_path_,
-      base::Bind(&NetworkSmsHandler::
-                 ModemManager1NetworkSmsDeviceHandler::ListCallback,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::BindOnce(&NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
+                         ListCallback,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::RequestUpdate() {
@@ -244,19 +245,23 @@ void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::RequestUpdate() {
   // implementation to deliver new sms messages.
   DBusThreadManager::Get()->GetModemMessagingClient()->List(
       std::string("AddSMS"), dbus::ObjectPath("/"),
-      base::Bind(&NetworkSmsHandler::
-                 ModemManager1NetworkSmsDeviceHandler::ListCallback,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::BindOnce(&NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
+                         ListCallback,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::ListCallback(
-    const std::vector<dbus::ObjectPath>& paths) {
+    base::Optional<std::vector<dbus::ObjectPath>> paths) {
   // This receives all messages, so clear any pending gets and deletes.
   retrieval_queue_.clear();
   delete_queue_.clear();
 
-  retrieval_queue_.resize(paths.size());
-  std::copy(paths.begin(), paths.end(), retrieval_queue_.begin());
+  if (!paths.has_value())
+    return;
+
+  retrieval_queue_.reserve(paths->size());
+  retrieval_queue_.assign(std::make_move_iterator(paths->begin()),
+                          std::make_move_iterator(paths->end()));
   if (!retrieving_messages_)
     GetMessages();
 }
@@ -270,13 +275,20 @@ void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::DeleteMessages() {
     return;
   }
   deleting_messages_ = true;
-  dbus::ObjectPath sms_path = delete_queue_.back();
+  dbus::ObjectPath sms_path = std::move(delete_queue_.back());
   delete_queue_.pop_back();
   DBusThreadManager::Get()->GetModemMessagingClient()->Delete(
       service_name_, object_path_, sms_path,
-      base::Bind(&NetworkSmsHandler::
-                 ModemManager1NetworkSmsDeviceHandler::DeleteMessages,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::BindOnce(&NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
+                         DeleteCallback,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::DeleteCallback(
+    bool success) {
+  if (!success)
+    return;
+  DeleteMessages();
 }
 
 // Messages must be fetched one at a time, so that we do not queue too