CrOS: Add success/failure indicators for biod methods with no callback.

chromeos/dbus/biod/biod_client.cc

Index: chromeos/dbus/biod/biod_client.cc
diff --git a/chromeos/dbus/biod/biod_client.cc b/chromeos/dbus/biod/biod_client.cc
index 5a77c18fdea6ae424d98a9be688f32859a6cfc43..8ea818dc915e1f1182841e056525edf59cbbf6ef 100644
--- a/chromeos/dbus/biod/biod_client.cc
+++ b/chromeos/dbus/biod/biod_client.cc
@@ -66,14 +66,17 @@ class BiodClientImpl : public BiodClient {
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void DestroyAllRecords() override {
+  void DestroyAllRecords(const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(
         biod::kBiometricsManagerInterface,
         biod::kBiometricsManagerDestroyAllRecordsMethod);
 
-    biod_proxy_->CallMethod(&method_call,
-                            dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                            dbus::ObjectProxy::EmptyResponseCallback());
+    biod_proxy_->CallMethodWithErrorCallback(

[Daniel Erat, 2017/04/08 00:12:34]

instead of using CallMethodWithErrorCallback, i think you can continue to use
CallMethod. it will log errors for you, and you can check whether the response
is null or not to decide if you should pass failure or success to the callback.

applies to all other methods in this change.

[sammiequon, 2017/04/08 01:03:30]

Hmm, ok. But when would call with error callback be preferred then?

[Daniel Erat, 2017/04/08 01:06:03]

you mean, when should CallMethodWithErrorCallback be used? it's mostly just
useful if the service actually includes additional information in its error
response and you want to be able to extract it.

[sammiequon, 2017/04/08 01:31:47]

Acknowledged.

+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&BiodClientImpl::OnVoidMethodSuccess,
+                   weak_ptr_factory_.GetWeakPtr(), callback),
+        base::Bind(&BiodClientImpl::OnVoidMethodFailure,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
   void StartAuthSession(const ObjectPathCallback& callback) override {
@@ -100,31 +103,39 @@ class BiodClientImpl : public BiodClient {
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void CancelEnrollSession(
-      const dbus::ObjectPath& enroll_session_path) override {
+  void CancelEnrollSession(const dbus::ObjectPath& enroll_session_path,
+                           const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kEnrollSessionInterface,
                                  biod::kEnrollSessionCancelMethod);
 
     dbus::ObjectProxy* enroll_session_proxy =
         bus_->GetObjectProxy(biod::kBiodServiceName, enroll_session_path);
-    enroll_session_proxy->CallMethod(
+    enroll_session_proxy->CallMethodWithErrorCallback(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        dbus::ObjectProxy::EmptyResponseCallback());
+        base::Bind(&BiodClientImpl::OnVoidMethodSuccess,
+                   weak_ptr_factory_.GetWeakPtr(), callback),
+        base::Bind(&BiodClientImpl::OnVoidMethodFailure,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void EndAuthSession(const dbus::ObjectPath& auth_session_path) override {
+  void EndAuthSession(const dbus::ObjectPath& auth_session_path,
+                      const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kAuthSessionInterface,
                                  biod::kAuthSessionEndMethod);
 
     dbus::ObjectProxy* auth_session_proxy =
         bus_->GetObjectProxy(biod::kBiodServiceName, auth_session_path);
-    auth_session_proxy->CallMethod(&method_call,
-                                   dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                                   dbus::ObjectProxy::EmptyResponseCallback());
+    auth_session_proxy->CallMethodWithErrorCallback(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&BiodClientImpl::OnVoidMethodSuccess,
+                   weak_ptr_factory_.GetWeakPtr(), callback),
+        base::Bind(&BiodClientImpl::OnVoidMethodFailure,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
   void SetRecordLabel(const dbus::ObjectPath& record_path,
-                      const std::string& label) override {
+                      const std::string& label,
+                      const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kRecordInterface,
                                  biod::kRecordSetLabelMethod);
     dbus::MessageWriter writer(&method_call);
@@ -137,15 +148,19 @@ class BiodClientImpl : public BiodClient {
                              dbus::ObjectProxy::EmptyResponseCallback());
   }
 
-  void RemoveRecord(const dbus::ObjectPath& record_path) override {
+  void RemoveRecord(const dbus::ObjectPath& record_path,
+                    const VoidDBusMethodCallback& callback) override {
     dbus::MethodCall method_call(biod::kRecordInterface,
                                  biod::kRecordRemoveMethod);
 
     dbus::ObjectProxy* record_proxy =
         bus_->GetObjectProxy(biod::kBiodServiceName, record_path);
-    record_proxy->CallMethod(&method_call,
-                             dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                             dbus::ObjectProxy::EmptyResponseCallback());
+    record_proxy->CallMethodWithErrorCallback(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&BiodClientImpl::OnVoidMethodSuccess,
+                   weak_ptr_factory_.GetWeakPtr(), callback),
+        base::Bind(&BiodClientImpl::OnVoidMethodFailure,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
   void RequestRecordLabel(const dbus::ObjectPath& record_path,
@@ -201,6 +216,29 @@ class BiodClientImpl : public BiodClient {
   }
 
  private:
+  void OnVoidMethodSuccess(const VoidDBusMethodCallback& callback,

[Daniel Erat, 2017/04/08 00:12:34]

this isn't accessing any of the class's state, so you can just move it into a
function in the anon namespace

[sammiequon, 2017/04/08 01:03:30]

Done.

+                           dbus::Response* response) {
+    if (!response) {

[Daniel Erat, 2017/04/08 00:12:34]

much simpler:

  callback.Run(response ? DBUS_METHOD_CALL_SUCCESS : DBUS_METHOD_CALL_FAILURE);

[sammiequon, 2017/04/08 01:03:30]

Done.

+      callback.Run(DBUS_METHOD_CALL_FAILURE);
+      return;
+    }
+    callback.Run(DBUS_METHOD_CALL_SUCCESS);
+  }
+
+  void OnVoidMethodFailure(const VoidDBusMethodCallback& callback,
+                           dbus::ErrorResponse* response) {
+    std::string error_name;
+    std::string error_message;
+    if (response) {
+      // Error message may contain the error message as string.
+      dbus::MessageReader reader(response);
+      error_name = response->GetErrorName();
+      reader.PopString(&error_message);
+    }
+    LOG(ERROR) << "Failed method call: " << error_name << ": " << error_message;
+    callback.Run(DBUS_METHOD_CALL_FAILURE);
+  }
+
   void OnStartEnrollSession(const ObjectPathCallback& callback,
                             dbus::Response* response) {
     dbus::ObjectPath result;