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

chromeos/dbus/biod/biod_client.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/dbus/biod/biod_client.h"
 
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/macros.h"
@@ skipping 48 matching lines @@
         biod::kBiometricsManagerGetRecordsForUserMethod);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(user_id);
 
     biod_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&BiodClientImpl::OnGetRecordsForUser,
                    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 {
     dbus::MethodCall method_call(
         biod::kBiometricsManagerInterface,
         biod::kBiometricsManagerStartAuthSessionMethod);
 
     biod_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&BiodClientImpl::OnStartAuthSession,
                    weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
   void RequestType(const BiometricTypeCallback& callback) override {
     dbus::MethodCall method_call(dbus::kDBusPropertiesInterface,
                                  dbus::kDBusPropertiesGet);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(biod::kBiometricsManagerInterface);
     writer.AppendString(biod::kBiometricsManagerBiometricTypeProperty);
 
     biod_proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&BiodClientImpl::OnRequestType,
                    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);
     writer.AppendString(label);
 
     dbus::ObjectProxy* record_proxy =
         bus_->GetObjectProxy(biod::kBiodServiceName, record_path);
     record_proxy->CallMethod(&method_call,
                              dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                              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,
                           const LabelCallback& callback) override {
     dbus::MethodCall method_call(dbus::kDBusPropertiesInterface,
                                  dbus::kDBusPropertiesGet);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(biod::kRecordInterface);
     writer.AppendString(biod::kRecordLabelProperty);
 
@@ skipping 35 matching lines @@
     biod_proxy_->ConnectToSignal(
         biod::kBiometricsManagerInterface,
         biod::kBiometricsManagerSessionFailedSignal,
         base::Bind(&BiodClientImpl::SessionFailedReceived,
                    weak_ptr_factory_.GetWeakPtr()),
         base::Bind(&BiodClientImpl::OnSignalConnected,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
  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;
     if (response) {
       dbus::MessageReader reader(response);
       if (!reader.PopObjectPath(&result)) {
         LOG(ERROR) << biod::kBiometricsManagerStartEnrollSessionMethod
                    << " had incorrect response.";
       }
     }
@@ skipping 144 matching lines @@
 
 // static
 BiodClient* BiodClient::Create(DBusClientImplementationType type) {
   if (type == REAL_DBUS_CLIENT_IMPLEMENTATION)
     return new BiodClientImpl();
   DCHECK_EQ(FAKE_DBUS_CLIENT_IMPLEMENTATION, type);
   return new FakeBiodClient();
 }
 
 }  // namespace chromeos