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

device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc

 // Copyright 2015 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 "device/bluetooth/bluetooth_remote_gatt_characteristic_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ skipping 118 matching lines @@
 
 bool BluetoothRemoteGattCharacteristicAndroid::UpdateValue(
     const std::vector<uint8_t>& value) {
   NOTIMPLEMENTED();
   return false;
 }
 
 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(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
   if (read_pending_ || write_pending_) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(error_callback,
                               BluetoothGattService::GATT_ERROR_IN_PROGRESS));
     return;
@@ skipping 41 matching lines @@
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::OnChanged(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jbyteArray>& value) {
   base::android::JavaByteArrayToByteVector(env, value, &value_);
   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,
     int32_t status,
     const JavaParamRef<jbyteArray>& value) {
   read_pending_ = false;
 
   // Clear callbacks before calling to avoid reentrancy issues.
   ValueCallback read_callback = read_callback_;
   ErrorCallback read_error_callback = read_error_callback_;
@@ skipping 62 matching lines @@
 void BluetoothRemoteGattCharacteristicAndroid::EnsureDescriptorsCreated()
     const {
   if (!descriptors_.empty())
     return;
 
   Java_ChromeBluetoothRemoteGattCharacteristic_createDescriptors(
       AttachCurrentThread(), j_characteristic_.obj());
 }
 
 }  // namespace device