Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

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 a269ff8819aaa70117888231d571ccb2aea6cb80..0e0944660a49d8d47888e9743685897c1e77a779 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
@@ -15,7 +15,6 @@
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/bluetooth/bluetooth_adapter_android.h"
-#include "device/bluetooth/bluetooth_gatt_notify_session_android.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor_android.h"
 #include "device/bluetooth/bluetooth_remote_gatt_service_android.h"
 #include "jni/ChromeBluetoothRemoteGattCharacteristic_jni.h"
@@ -52,11 +51,6 @@ BluetoothRemoteGattCharacteristicAndroid::
     ~BluetoothRemoteGattCharacteristicAndroid() {
   Java_ChromeBluetoothRemoteGattCharacteristic_onBluetoothRemoteGattCharacteristicAndroidDestruction(
       AttachCurrentThread(), j_characteristic_.obj());
-
-  if (pending_start_notify_calls_.size()) {
-    OnStartNotifySessionError(
-        device::BluetoothRemoteGattService::GATT_ERROR_FAILED);
-  }
 }
 
 // static
@@ -102,11 +96,6 @@ BluetoothRemoteGattCharacteristicAndroid::GetPermissions() const {
   return 0;
 }
 
-bool BluetoothRemoteGattCharacteristicAndroid::IsNotifying() const {
-  NOTIMPLEMENTED();
-  return false;
-}
-
 std::vector<BluetoothRemoteGattDescriptor*>
 BluetoothRemoteGattCharacteristicAndroid::GetDescriptors() const {
   EnsureDescriptorsCreated();
@@ -126,68 +115,6 @@ BluetoothRemoteGattCharacteristicAndroid::GetDescriptor(
   return iter->second;
 }
 
-void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
-    const NotifySessionCallback& callback,
-    const ErrorCallback& error_callback) {
-  if (!pending_start_notify_calls_.empty()) {
-    pending_start_notify_calls_.push_back(
-        std::make_pair(callback, error_callback));
-    return;
-  }
-
-  Properties properties = GetProperties();
-
-  bool hasNotify = properties & PROPERTY_NOTIFY;
-  bool hasIndicate = properties & PROPERTY_INDICATE;
-
-  if (!hasNotify && !hasIndicate) {
-    LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(error_callback,
-                   BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
-    return;
-  }
-
-  std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
-      GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
-                               ClientCharacteristicConfigurationUuid());
-
-  if (ccc_descriptor.size() != 1u) {
-    LOG(ERROR) << "Found " << ccc_descriptor.size()
-               << " client characteristic configuration descriptors.";
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(error_callback,
-                   (ccc_descriptor.size() == 0)
-                       ? BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED
-                       : BluetoothRemoteGattService::GATT_ERROR_FAILED));
-    return;
-  }
-
-  if (!Java_ChromeBluetoothRemoteGattCharacteristic_setCharacteristicNotification(
-          AttachCurrentThread(), j_characteristic_.obj(), true)) {
-    LOG(ERROR) << "Error enabling characteristic notification";
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(error_callback,
-                              BluetoothRemoteGattService::GATT_ERROR_FAILED));
-    return;
-  }
-
-  std::vector<uint8_t> value(2);
-  value[0] = hasNotify ? 1 : 2;
-
-  pending_start_notify_calls_.push_back(
-      std::make_pair(callback, error_callback));
-  ccc_descriptor[0]->WriteRemoteDescriptor(
-      value, base::Bind(&BluetoothRemoteGattCharacteristicAndroid::
-                            OnStartNotifySessionSuccess,
-                        base::Unretained(this)),
-      base::Bind(
-          &BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionError,
-          base::Unretained(this)));
-}
-
 void BluetoothRemoteGattCharacteristicAndroid::ReadRemoteCharacteristic(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
@@ -247,27 +174,6 @@ void BluetoothRemoteGattCharacteristicAndroid::OnChanged(
   adapter_->NotifyGattCharacteristicValueChanged(this, value_);
 }
 
-void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionSuccess() {
-  std::vector<PendingStartNotifyCall> reentrant_safe_callbacks;
-  reentrant_safe_callbacks.swap(pending_start_notify_calls_);
-
-  for (const auto& callback_pair : reentrant_safe_callbacks) {
-    std::unique_ptr<device::BluetoothGattNotifySession> notify_session(
-        new BluetoothGattNotifySessionAndroid(instance_id_));
-    callback_pair.first.Run(std::move(notify_session));
-  }
-}
-
-void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionError(
-    BluetoothRemoteGattService::GattErrorCode error) {
-  std::vector<PendingStartNotifyCall> reentrant_safe_callbacks;
-  reentrant_safe_callbacks.swap(pending_start_notify_calls_);
-
-  for (auto const& callback_pair : reentrant_safe_callbacks) {
-    callback_pair.second.Run(error);
-  }
-}
-
 void BluetoothRemoteGattCharacteristicAndroid::OnRead(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
@@ -333,6 +239,87 @@ void BluetoothRemoteGattCharacteristicAndroid::CreateGattRemoteDescriptor(
                        chrome_bluetooth_device));
 }
 
+void BluetoothRemoteGattCharacteristicAndroid::SubscribeToNotifications(

[ortuno, 2016/07/28 21:59:30]

I would be OK with keeping this as NOTIMPLEMENTED in this CL. And moving each
platform in a different CL.

[tommyt, 2016/08/01 11:39:21]

Would you also be OK with leaving it here? With my latest patchset, there is
very little code left in this method.

+    const base::Closure& callback,
+    const ErrorCallback& error_callback) {
+  Properties properties = GetProperties();
+
+  bool hasNotify = properties & PROPERTY_NOTIFY;
+  bool hasIndicate = properties & PROPERTY_INDICATE;
+
+  if (!hasNotify && !hasIndicate) {
+    LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
+    return;
+  }
+
+  std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
+      GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
+                               ClientCharacteristicConfigurationUuid());
+
+  if (ccc_descriptor.size() != 1u) {

[ortuno, 2016/07/28 21:59:30]

I think we should move this to the cross platform implementation as well. See
comment in bluetooth_remote_gatt_characteristic.h

[tommyt, 2016/08/01 11:39:21]

Done.

+    LOG(ERROR) << "Found " << ccc_descriptor.size()
+               << " client characteristic configuration descriptors.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   (ccc_descriptor.size() == 0)
+                       ? BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED
+                       : BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  if (!Java_ChromeBluetoothRemoteGattCharacteristic_setCharacteristicNotification(
+          AttachCurrentThread(), j_characteristic_.obj(), true)) {
+    LOG(ERROR) << "Error enabling characteristic notification";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(error_callback,
+                              BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  std::vector<uint8_t> value(2);

[ortuno, 2016/07/28 21:59:30]

optional nit:

std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

[tommyt, 2016/08/01 11:39:22]

This doesn't seem to compile...

+  value[0] = hasNotify ? 1 : 2;
+
+  ccc_descriptor[0]->WriteRemoteDescriptor(value, callback, error_callback);
+}
+
+void BluetoothRemoteGattCharacteristicAndroid::UnsubscribeFromNotifications(
+    const base::Closure& callback,
+    const ErrorCallback& error_callback) {
+  std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
+      GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
+                               ClientCharacteristicConfigurationUuid());
+
+  if (ccc_descriptor.size() != 1u) {
+    LOG(ERROR) << "Found " << ccc_descriptor.size()
+               << " client characteristic configuration descriptors.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   device::BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  if (!Java_ChromeBluetoothRemoteGattCharacteristic_setCharacteristicNotification(
+          AttachCurrentThread(), j_characteristic_.obj(), false)) {
+    LOG(ERROR) << "Error disabling characteristic notification";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   device::BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  std::vector<uint8_t> value(2);
+  value[0] = 0;
+
+  ccc_descriptor[0]->WriteRemoteDescriptor(value, callback, error_callback);
+}
+
 BluetoothRemoteGattCharacteristicAndroid::
     BluetoothRemoteGattCharacteristicAndroid(
         BluetoothAdapterAndroid* adapter,