bluetooth: Make in progress errors consistent across android

device/bluetooth/bluetooth_remote_gatt_characteristic.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
index 9443ba97abd8ec698c24f5ca1fca2e0b0323c610..2bf85c756655d56a73af3ec0b7111fc32960149b 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
@@ -8,6 +8,7 @@
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "device/bluetooth/bluetooth_device.h"
 #include "device/bluetooth/bluetooth_gatt_notify_session.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor.h"
 
@@ -85,6 +86,33 @@ void BluetoothRemoteGattCharacteristic::StartNotifySession(
   }
 }
 
+void BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristic(

[scheib, 2017/04/21 23:33:10]

Consider keeping all logic in the base classes, never referring to
PendingGattOperation in the platform specific sub-classes. Ideally the sub class
___Impl methods do the minimal OS specific work to perform the task and then
fire back the callbacks.

The methods on device would check and set the pending_gatt_operation_ state, and
wrap callbacks so that when the sub-classes Run them they clear
pending_gatt_operation_.

There may be benefit from having generic methods

something roughly like:

BluetoothRemoteGattCharacteristic::ReadRemoteCharacteristic(callback,
error_callback) {
  if (ReserveGattOperationOrRunError(error_callback)) {
    ReadRemoteCharacteristicImpl(
        MakeValueCallbackAfterGattOperation(callback),
        MakeErrorCallbackAfterGattOperation(error_callback));
  }

where MakeValueCallbackAfterGattOperation come from BluetoothDevice and
generically return a callback that wraps another callback and clears
pending_gatt_operation_ before running the inner callback.

[ortuno, 2017/04/28 03:47:12]

I was exploring something like that at some point albeit only at the
characteristic level. I ended up abandoning that approach because it could
affect reentrancy and we have no tests for it. Since this is somewhat orthogonal
I'll do it in a separate CL before continuing this.

+    const ValueCallback& callback,
+    const ErrorCallback& error_callback) {
+  if (HasPendingGattOperation()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  ReadRemoteCharacteristicImpl(callback, error_callback);
+}
+
+void BluetoothRemoteGattCharacteristic::WriteRemoteCharacteristic(
+    const std::vector<uint8_t>& value,
+    const base::Closure& callback,
+    const ErrorCallback& error_callback) {
+  if (HasPendingGattOperation()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  WriteRemoteCharacteristicImpl(value, callback, error_callback);
+}
+
 void BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession(
     NotifySessionCallback callback,
     ErrorCallback error_callback,
@@ -228,6 +256,13 @@ void BluetoothRemoteGattCharacteristic::OnStartNotifySessionError(
   }
 }
 
+bool BluetoothRemoteGattCharacteristic::HasPendingGattOperation() {
+  return GetService()->GetDevice()->HasPendingGattOperation();
+}
+void BluetoothRemoteGattCharacteristic::SetPendingGattOperation(bool pending) {
+  GetService()->GetDevice()->SetPendingGattOperation(pending);
+}
+
 void BluetoothRemoteGattCharacteristic::StopNotifySession(
     BluetoothGattNotifySession* session,
     const base::Closure& callback) {