Migrate GsmSMSClient to DBusCallback.

BUG=739622
TEST=Ran chromeos_unittests.

Change-Id: Ic79e5491ccf36e2cb83642545dd5b29763c0eedc
Reviewed-on: https://chromium-review.googlesource.com/903607Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534711}

chromeos/dbus/fake_gsm_sms_client.cc

@@ -50,28 +50,31 @@ void FakeGsmSMSClient::ResetSmsReceivedHandler(
 void FakeGsmSMSClient::Delete(const std::string& service_name,
                               const dbus::ObjectPath& object_path,
                               uint32_t index,
-                              const DeleteCallback& callback) {
-  message_list_.Remove(index, NULL);
-  callback.Run();
+                              VoidDBusMethodCallback callback) {
+  message_list_.Remove(index, nullptr);
+  std::move(callback).Run(true);
 }
 
 void FakeGsmSMSClient::Get(const std::string& service_name,
                            const dbus::ObjectPath& object_path,
                            uint32_t index,
-                           const GetCallback& callback) {
-  base::DictionaryValue* dictionary = NULL;
-  if (message_list_.GetDictionary(index, &dictionary)) {
-    callback.Run(*dictionary);
+                           DBusMethodCallback<base::DictionaryValue> callback) {
+  base::DictionaryValue* dictionary = nullptr;
+  if (!message_list_.GetDictionary(index, &dictionary)) {
+    std::move(callback).Run(base::nullopt);
     return;
   }
-  base::DictionaryValue empty_dictionary;
-  callback.Run(empty_dictionary);
+
+  // TODO(crbug.com/646113): Once migration is done, this can be simplified.
+  base::DictionaryValue copy;
+  copy.MergeDictionary(dictionary);
+  std::move(callback).Run(std::move(copy));
 }
 
 void FakeGsmSMSClient::List(const std::string& service_name,
                             const dbus::ObjectPath& object_path,
-                            const ListCallback& callback) {
-  callback.Run(message_list_);
+                            DBusMethodCallback<base::ListValue> callback) {
+  std::move(callback).Run(base::ListValue(message_list_.GetList()));
 }
 
 void FakeGsmSMSClient::RequestUpdate(const std::string& service_name,

chromeos/dbus/fake_gsm_sms_client.h

@@ -34,14 +34,14 @@ class CHROMEOS_EXPORT FakeGsmSMSClient : public GsmSMSClient {
   void Delete(const std::string& service_name,
               const dbus::ObjectPath& object_path,
               uint32_t index,
-              const DeleteCallback& callback) override;
+              VoidDBusMethodCallback callback) override;
   void Get(const std::string& service_name,
            const dbus::ObjectPath& object_path,
            uint32_t index,
-           const GetCallback& callback) override;
+           DBusMethodCallback<base::DictionaryValue> callback) override;
   void List(const std::string& service_name,
             const dbus::ObjectPath& object_path,
-            const ListCallback& callback) override;
+            DBusMethodCallback<base::ListValue> callback) override;
   void RequestUpdate(const std::string& service_name,
                      const dbus::ObjectPath& object_path) override;
 

chromeos/dbus/gsm_sms_client.cc

@@ -31,9 +31,6 @@ namespace {
 class SMSProxy {
  public:
   typedef GsmSMSClient::SmsReceivedHandler SmsReceivedHandler;
-  typedef GsmSMSClient::DeleteCallback DeleteCallback;
-  typedef GsmSMSClient::GetCallback GetCallback;
-  typedef GsmSMSClient::ListCallback ListCallback;
 
   SMSProxy(dbus::Bus* bus,
            const std::string& service_name,
@@ -60,7 +57,7 @@ class SMSProxy {
   }
 
   // Calls Delete method.
-  void Delete(uint32_t index, const DeleteCallback& callback) {
+  void Delete(uint32_t index, VoidDBusMethodCallback callback) {
     dbus::MethodCall method_call(modemmanager::kModemManagerSMSInterface,
                                  modemmanager::kSMSDeleteFunction);
     dbus::MessageWriter writer(&method_call);
@@ -68,11 +65,11 @@ class SMSProxy {
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&SMSProxy::OnDelete, weak_ptr_factory_.GetWeakPtr(),
-                       callback));
+                       std::move(callback)));
   }
 
   // Calls Get method.
-  void Get(uint32_t index, const GetCallback& callback) {
+  void Get(uint32_t index, DBusMethodCallback<base::DictionaryValue> callback) {
     dbus::MethodCall method_call(modemmanager::kModemManagerSMSInterface,
                                  modemmanager::kSMSGetFunction);
     dbus::MessageWriter writer(&method_call);
@@ -80,17 +77,17 @@ class SMSProxy {
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&SMSProxy::OnGet, weak_ptr_factory_.GetWeakPtr(),
-                       callback));
+                       std::move(callback)));
   }
 
   // Calls List method.
-  void List(const ListCallback& callback) {
+  void List(DBusMethodCallback<base::ListValue> callback) {
     dbus::MethodCall method_call(modemmanager::kModemManagerSMSInterface,
                                  modemmanager::kSMSListFunction);
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::BindOnce(&SMSProxy::OnList, weak_ptr_factory_.GetWeakPtr(),
-                       callback));
+                       std::move(callback)));
   }
 
  private:
@@ -117,38 +114,42 @@ class SMSProxy {
   }
 
   // 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 Get method calls.
-  void OnGet(const GetCallback& callback, dbus::Response* response) {
-    if (!response)
+  void OnGet(DBusMethodCallback<base::DictionaryValue> callback,
+             dbus::Response* response) {
+    if (!response) {
+      std::move(callback).Run(base::nullopt);
       return;
+    }
     dbus::MessageReader reader(response);
-    std::unique_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
-    base::DictionaryValue* dictionary_value = NULL;
-    if (!value.get() || !value->GetAsDictionary(&dictionary_value)) {
+    auto value = base::DictionaryValue::From(dbus::PopDataAsValue(&reader));
+    if (!value) {
       LOG(WARNING) << "Invalid response: " << response->ToString();
+      std::move(callback).Run(base::nullopt);
       return;
     }
-    callback.Run(*dictionary_value);
+    std::move(callback).Run(std::move(*value));
   }
 
   // Handles responses of List method calls.
-  void OnList(const ListCallback& callback, dbus::Response* response) {
-    if (!response)
+  void OnList(DBusMethodCallback<base::ListValue> callback,
+              dbus::Response* response) {
+    if (!response) {
+      std::move(callback).Run(base::nullopt);
       return;
+    }
     dbus::MessageReader reader(response);
-    std::unique_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
-    base::ListValue* list_value = NULL;
-    if (!value.get() || !value->GetAsList(&list_value)) {
+    auto value = base::ListValue::From(dbus::PopDataAsValue(&reader));
+    if (!value) {
       LOG(WARNING) << "Invalid response: " << response->ToString();
+      std::move(callback).Run(base::nullopt);
       return;
     }
-    callback.Run(*list_value);
+    std::move(callback).Run(std::move(*value));
   }
 
   dbus::ObjectProxy* proxy_;
@@ -183,23 +184,23 @@ class GsmSMSClientImpl : public GsmSMSClient {
   void Delete(const std::string& service_name,
               const dbus::ObjectPath& object_path,
               uint32_t index,
-              const DeleteCallback& callback) override {
-    GetProxy(service_name, object_path)->Delete(index, callback);
+              VoidDBusMethodCallback callback) override {
+    GetProxy(service_name, object_path)->Delete(index, std::move(callback));
   }
 
   // GsmSMSClient override.
   void Get(const std::string& service_name,
            const dbus::ObjectPath& object_path,
            uint32_t index,
-           const GetCallback& callback) override {
-    GetProxy(service_name, object_path)->Get(index, callback);
+           DBusMethodCallback<base::DictionaryValue> callback) override {
+    GetProxy(service_name, object_path)->Get(index, std::move(callback));
   }
 
   // GsmSMSClient override.
   void List(const std::string& service_name,
             const dbus::ObjectPath& object_path,
-            const ListCallback& callback) override {
-    GetProxy(service_name, object_path)->List(callback);
+            DBusMethodCallback<base::ListValue> callback) override {
+    GetProxy(service_name, object_path)->List(std::move(callback));
   }
 
   // GsmSMSClient override.

chromeos/dbus/gsm_sms_client.h

@@ -13,6 +13,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 base {
 class DictionaryValue;
@@ -33,9 +34,6 @@ class CHROMEOS_EXPORT GsmSMSClient : public DBusClient {
  public:
   typedef base::Callback<void(uint32_t index, bool complete)>
       SmsReceivedHandler;
-  typedef base::Callback<void()> DeleteCallback;
-  typedef base::Callback<void(const base::DictionaryValue& sms)> GetCallback;
-  typedef base::Callback<void(const base::ListValue& result)> ListCallback;
 
   ~GsmSMSClient() override;
 
@@ -52,22 +50,22 @@ class CHROMEOS_EXPORT GsmSMSClient : 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 call completion.
   virtual void Delete(const std::string& service_name,
                       const dbus::ObjectPath& object_path,
                       uint32_t index,
-                      const DeleteCallback& callback) = 0;
+                      VoidDBusMethodCallback callback) = 0;
 
-  // Calls Get method.  |callback| is called after the method call succeeds.
+  // Calls Get method.  |callback| is called on method call completion.
   virtual void Get(const std::string& service_name,
                    const dbus::ObjectPath& object_path,
                    uint32_t index,
-                   const GetCallback& callback) = 0;
+                   DBusMethodCallback<base::DictionaryValue> callback) = 0;
 
-  // Calls List method.  |callback| is called after the method call succeeds.
+  // Calls List method.  |callback| is called on method call completion.
   virtual void List(const std::string& service_name,
                     const dbus::ObjectPath& object_path,
-                    const ListCallback& callback) = 0;
+                    DBusMethodCallback<base::ListValue> callback) = 0;
 
   // Requests a check for new messages. In shill this does nothing. The
   // stub implementation uses it to generate a sequence of test messages.

chromeos/dbus/gsm_sms_client_unittest.cc

@@ -222,13 +222,16 @@ TEST_F(GsmSMSClientTest, Delete) {
   response_ = response.get();
 
   // Call Delete.
-  bool called = false;
-  client_->Delete(kServiceName, dbus::ObjectPath(kObjectPath), kIndex,
-                  base::Bind([](bool* called) { *called = true; }, &called));
+  base::Optional<bool> success;
+  client_->Delete(
+      kServiceName, dbus::ObjectPath(kObjectPath), kIndex,
+      base::BindOnce([](base::Optional<bool>* success_out,
+                        bool success) { success_out->emplace(success); },
+                     &success));
 
   // Run the message loop.
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(called);
+  EXPECT_EQ(true, success);
 }
 
 TEST_F(GsmSMSClientTest, Get) {
@@ -255,32 +258,23 @@ TEST_F(GsmSMSClientTest, Get) {
   response_ = response.get();
 
   // Call Get.
-  base::Value raw_result;
-  client_->Get(
-      kServiceName, dbus::ObjectPath(kObjectPath), kIndex,
-      base::Bind(
-          [](base::Value* result_out, const base::DictionaryValue& result) {
-            *result_out = result.Clone();
-          },
-          &raw_result));
+  base::Optional<base::DictionaryValue> result;
+  client_->Get(kServiceName, dbus::ObjectPath(kObjectPath), kIndex,
+               base::BindOnce(
+                   [](base::Optional<base::DictionaryValue>* result_out,
+                      base::Optional<base::DictionaryValue> result) {
+                     *result_out = std::move(result);
+                   },
+                   &result));
 
   // Run the message loop.
   base::RunLoop().RunUntilIdle();
 
   // Verify the result.
-  const auto result = base::DictionaryValue::From(
-      base::Value::ToUniquePtrValue(std::move(raw_result)));
-  ASSERT_TRUE(result);
-  EXPECT_EQ(2u, result->size());
-
-  const base::Value* number =
-      result->FindKeyOfType(kNumberKey, base::Value::Type::STRING);
-  ASSERT_TRUE(number);
-  EXPECT_EQ(kExampleNumber, number->GetString());
-  const base::Value* text =
-      result->FindKeyOfType(kTextKey, base::Value::Type::STRING);
-  ASSERT_TRUE(text);
-  EXPECT_EQ(kExampleText, text->GetString());
+  base::DictionaryValue expected_result;
+  expected_result.SetKey(kNumberKey, base::Value(kExampleNumber));
+  expected_result.SetKey(kTextKey, base::Value(kExampleText));
+  EXPECT_EQ(expected_result, result);
 }
 
 TEST_F(GsmSMSClientTest, List) {
@@ -308,11 +302,12 @@ TEST_F(GsmSMSClientTest, List) {
   response_ = response.get();
 
   // Call List.
-  base::Value result;
+  base::Optional<base::ListValue> result;
   client_->List(kServiceName, dbus::ObjectPath(kObjectPath),
-                base::Bind(
-                    [](base::Value* result_out, const base::ListValue& result) {
-                      *result_out = result.Clone();
+                base::BindOnce(
+                    [](base::Optional<base::ListValue>* result_out,
+                       base::Optional<base::ListValue> result) {
+                      *result_out = std::move(result);
                     },
                     &result));
 

chromeos/network/network_sms_handler.cc

@@ -59,10 +59,12 @@ class NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler
   void RequestUpdate() override;
 
  private:
-  void ListCallback(const base::ListValue& message_list);
+  void ListCallback(base::Optional<base::ListValue> message_list);
   void SmsReceivedCallback(uint32_t index, bool complete);
-  void GetCallback(uint32_t index, const base::DictionaryValue& dictionary);
+  void GetCallback(uint32_t index,
+                   base::Optional<base::DictionaryValue> dictionary);
   void DeleteMessages();
+  void DeleteCallback(bool success);
   void MessageReceived(const base::DictionaryValue& dictionary);
 
   NetworkSmsHandler* host_;
@@ -105,13 +107,15 @@ void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::RequestUpdate() {
 }
 
 void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::ListCallback(
-    const base::ListValue& message_list) {
+    base::Optional<base::ListValue> message_list) {
+  if (!message_list.has_value())
+    return;
+
   // This receives all messages, so clear any pending deletes.
   delete_queue_.clear();
-  for (base::ListValue::const_iterator iter = message_list.begin();
-       iter != message_list.end(); ++iter) {
-    const base::DictionaryValue* message = NULL;
-    if (iter->GetAsDictionary(&message))
+  for (const auto& entry : message_list.value()) {
+    const base::DictionaryValue* message = nullptr;
+    if (entry.GetAsDictionary(&message))
       continue;
     MessageReceived(*message);
     double index = 0;
@@ -134,9 +138,16 @@ void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::DeleteMessages() {
   delete_queue_.pop_back();
   DBusThreadManager::Get()->GetGsmSMSClient()->Delete(
       service_name_, object_path_, index,
-      base::Bind(&NetworkSmsHandler::
-                 ModemManagerNetworkSmsDeviceHandler::DeleteMessages,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::BindOnce(&NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::
+                         DeleteCallback,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::DeleteCallback(
+    bool success) {
+  if (!success)
+    return;
+  DeleteMessages();
 }
 
 void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::
@@ -146,15 +157,18 @@ void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::
     return;
   DBusThreadManager::Get()->GetGsmSMSClient()->Get(
       service_name_, object_path_, index,
-      base::Bind(&NetworkSmsHandler::
-                 ModemManagerNetworkSmsDeviceHandler::GetCallback,
-                 weak_ptr_factory_.GetWeakPtr(), index));
+      base::BindOnce(
+          &NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::GetCallback,
+          weak_ptr_factory_.GetWeakPtr(), index));
 }
 
 void NetworkSmsHandler::ModemManagerNetworkSmsDeviceHandler::GetCallback(
     uint32_t index,
-    const base::DictionaryValue& dictionary) {
-  MessageReceived(dictionary);
+    base::Optional<base::DictionaryValue> dictionary) {
+  if (!dictionary.has_value())
+    return;
+
+  MessageReceived(dictionary.value());
   delete_queue_.push_back(index);
   if (!deleting_messages_)
     DeleteMessages();