bluetooth: android: Confirm the notify session after the descriptor has been written.

device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
index d8d029900367defb7a0fc01af7d8895132b876fe..53e9509a4404caabf16936c783be82aacb2b71ab 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
@@ -136,20 +136,18 @@ bool BluetoothRemoteGattCharacteristicAndroid::UpdateValue(
 void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
     const NotifySessionCallback& callback,
     const ErrorCallback& error_callback) {
-  if (Java_ChromeBluetoothRemoteGattCharacteristic_startNotifySession(
-          AttachCurrentThread(), j_characteristic_.obj())) {
-    // TODO(crbug.com/569664): Wait until descriptor write completes before
-    // reporting success via calling |callback|.
-    scoped_ptr<device::BluetoothGattNotifySession> notify_session(
-        new BluetoothGattNotifySessionAndroid(instance_id_));
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, base::Passed(&notify_session)));
-  } else {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(error_callback,
-                   BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
+  if (pending_start_notify_calls_.empty()) {

[scheib, 2016/02/26 04:27:01]

Starting notify, reading & writing the CCC descriptor operations are all
mutually exclusive. Please add a check that the CCC descriptor doesn't have an
in-flight read or write operation. And when notify is being started please flag
the CCC descriptor to reject any read/write.

It's possible that the logic that does the descriptor write in Java should just
be moved to C++ here.  Then the Java code wouldn't even need to know that the
start notify was pending, etc. It would just be a 'normal' descriptor write. 
And the descriptor C++ code would just guard as already implemented for
read|write in progress.

Either way, please also add tests for startnotify and read/write operations
colliding.

[tommyt, 2016/03/01 14:45:14]

I agree with moving this to C++. My newest changeset does this, and I think it's
quite a bit nicer.

+    if (!Java_ChromeBluetoothRemoteGattCharacteristic_startNotifySession(
+            AttachCurrentThread(), j_characteristic_.obj())) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(error_callback,
+                     BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
+      return;
+    }
   }
+
+  pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::ReadRemoteCharacteristic(
@@ -211,6 +209,24 @@ void BluetoothRemoteGattCharacteristicAndroid::OnChanged(
   adapter_->NotifyGattCharacteristicValueChanged(this, value_);
 }
 
+void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySession(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& jcaller,
+    int32_t status) {
+  while (!pending_start_notify_calls_.empty()) {
+    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
+    pending_start_notify_calls_.pop();
+    if (status == 0) {  // android.bluetooth.BluetoothGatt.GATT_SUCCESS
+      scoped_ptr<device::BluetoothGattNotifySession> notify_session(
+          new BluetoothGattNotifySessionAndroid(instance_id_));
+      callbacks.first.Run(std::move(notify_session));
+    } else {
+      callbacks.second.Run(
+          BluetoothRemoteGattServiceAndroid::GetGattErrorCode(status));
+    }
+  }
+}
+
 void BluetoothRemoteGattCharacteristicAndroid::OnRead(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,