Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

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 fa96a9b984152d6c57ad23a0b17cd390ce434e7c..174512281f155e25bb4b7af3885681f1e1a100ad 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
@@ -4,17 +4,27 @@
 
 #include "device/bluetooth/bluetooth_remote_gatt_characteristic.h"
 
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "device/bluetooth/bluetooth_gatt_notify_session.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor.h"
 
 namespace device {
 
-BluetoothRemoteGattCharacteristic::BluetoothRemoteGattCharacteristic() {}
+BluetoothRemoteGattCharacteristic::BluetoothRemoteGattCharacteristic()
+    : weak_ptr_factory_(this) {}
 
-BluetoothRemoteGattCharacteristic::~BluetoothRemoteGattCharacteristic() {}
+BluetoothRemoteGattCharacteristic::~BluetoothRemoteGattCharacteristic() {
+  while (!pending_notify_commands_.empty()) {
+    pending_notify_commands_.front()->Cancel();
+  }
+}
 
 std::vector<BluetoothRemoteGattDescriptor*>
 BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID(
-    const BluetoothUUID& uuid) {
+    const BluetoothUUID& uuid) const {
   std::vector<BluetoothRemoteGattDescriptor*> descriptors;
   for (BluetoothRemoteGattDescriptor* descriptor : GetDescriptors()) {
     if (descriptor->GetUUID() == uuid) {
@@ -24,4 +34,221 @@ BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID(
   return descriptors;
 }
 
+base::WeakPtr<device::BluetoothRemoteGattCharacteristic>
+BluetoothRemoteGattCharacteristic::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+bool BluetoothRemoteGattCharacteristic::IsNotifying() const {
+  return !notify_sessions_.empty();
+}
+
+void BluetoothRemoteGattCharacteristic::StartNotifySession(

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

This looks very similar to what I first wrote for the notifications queue in the
renderer and also what I started to do for the getPrimaryServices code. I think
we can simplify this a bit by using closures so that we don't have to create to
a base class and two classes that implement it. What do you think of the
following code? It introduces a NotifySessionTask that takes a closure and a
callback to be called when cancelling tasks. The rest of the code is very
similar to what you have already.


NotifySessionTask(Closure task, ErrorCallback error_callback) 
  : task_(task),
    error_callback_(error_callback) {
}

~NotifySessionTask() {}

NotifySessionTask::Run() {
  task.Run();
}

NotifySessionTask::Cancel() {
  error_callback_.Run(GATT_ERROR);
}

StartNotifySessionImpl(callback, error_callback) {
   if (IsNotifying()) {
    base::ThreadTaskRunnerHandle::Get()->PostTask(
        FROM_HERE, base::Bind(&OnSubscribeToNotificationsSuccess,
                              weak_ptr_factory_.GetWeakPtr(),
                              callback, error_callback));
    return;
  }
  SubscribeToNotifications(base::Bind(OnSubscribeToNotificationsSuccess,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      callback),
                           base::Bind(OnSubscribetoNotificationsFailed,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      error_callback));
}

// TODO: Implement StopNotifySessionImpl


OnSubscribeToNotificationsSuccess(NotifySessionCallback callback) {
  pending_notify_tasks_.pop();
  // TODO: make notify session.
  callback.Run(std::move(notify_session));
  
  if (!pending_notify_tasks_.empty()) {
    pending_notify_tasks_.front().Run();
  }
}

// TODO: Implement OnSubscribeToNotificationsSuccess,
OnSubscribeToNotificationsError,
// OnUnsubscribeToNotificationsSuccess and OnUnsubscribeToNotificationsError.

StartNotifySession(NotifySessionCallback callback, ErrorCallback,
error_callback) {
  pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
    base::Bind(StartNotifySessionImpl, this, callback, error_callback),
error_callback));
  if (pending_notify_tasks_size() == 1) {
    pending_notify_tasks_.front().Run();
  }
}

StopNotifySession(...) {
  pending_notify_tasks_.push(MakeUnique<NotifySessionTask>(
    base::Bind(StopNotifySessionImpl, callback, error_callback),
error_callback));
  if (pending_notify_tasks_size() == 1) {
    pending_notify_tasks_.fron().Run();
  }
}

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

I really like this approach, and I agree that the multiple Command classes is
not super-elegant. The only problem is that the success callbacks for
StartNotifySession and StopNotifySession are not the same type (The former takes
a session object as a parameter, the latter does not have any parameters). I
think it might be easier to do this change at the same time as we move to
creating session objects immediately in StartNotifySession without waiting for
success or error. Once we make that change, the success callbacks can be plain
Closures for both start and stop.

[ortuno, 2016/08/02 01:56:31]

Hmm maybe I'm missing something, but I don't think the different callback type
makes a difference? You aren't storing the actual callback just a closure to run
StartNotifySessionImpl or StopNotifySessionImpl.

[ortuno, 2016/08/03 16:55:42]

Ah I see what you mean now. You could still just save a closure with the error
e.g.:

new NotifySessionTask(base::Bind(StartNotifySessionImpl,...),
base::Bind(error_callback, GATT_ERROR));

And for Stop:

new NotifySessionTask(base::Bind(StopNotifySessionImpl,...), callback);

[tommyt, 2016/08/08 13:55:08]

Done.

+    const NotifySessionCallback& callback,
+    const ErrorCallback& error_callback) {
+  NotifySessionCommand* command =
+      new NotifySessionCommandStart(this, callback, error_callback);
+  pending_notify_commands_.push(std::unique_ptr<NotifySessionCommand>(command));
+  if (pending_notify_commands_.size() == 1) {
+    command->Execute();
+  }
+}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommand::NotifySessionCommand(
+    BluetoothRemoteGattCharacteristic* characteristic,
+    Type type)
+    : characteristic_(characteristic), type_(type) {}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommand::
+    ~NotifySessionCommand() {}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::
+    NotifySessionCommandStart(BluetoothRemoteGattCharacteristic* characteristic,
+                              const NotifySessionCallback& callback,
+                              const ErrorCallback& error_callback)
+    : NotifySessionCommand(characteristic, COMMAND_START),
+      callback_(callback),
+      error_callback_(error_callback) {}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::
+    ~NotifySessionCommandStart() {}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::Execute() {
+  Properties properties = characteristic_->GetProperties();
+
+  bool hasNotify = (properties & PROPERTY_NOTIFY) != 0;
+  bool hasIndicate = (properties & PROPERTY_INDICATE) != 0;
+
+  if (!hasNotify && !hasIndicate) {

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

I think you can move this logic into SubscribeToNotifications. See comment in
SubscribeToNotifications.

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

Actually, I'll be moving more functionality into this function. This function is
cross-platform, and I think this is an ok place to keep all of this logic.

[ortuno, 2016/08/02 01:56:31]

I'm a bit hesitant to put low level concepts like properties and descriptors in
a high level function that is meant to coordinate notify sessions.

[tommyt, 2016/08/08 13:55:08]

This has been changed a bit in the latest changeset. This code  is now contained
in BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession().

+    LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::
+                       NotifySessionCommandStart::OnError,
+                   base::Unretained(this),
+                   BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
+    return;
+  }
+
+  if (characteristic_->IsNotifying()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&BluetoothRemoteGattCharacteristic::
+                                  NotifySessionCommandStart::OnSuccess,
+                              base::Unretained(this)));
+    return;
+  }
+
+  characteristic_->SubscribeToNotifications(
+      base::Bind(&BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::
+                     OnSuccess,
+                 base::Unretained(this)),
+      base::Bind(&BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::
+                     OnError,
+                 base::Unretained(this)));
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::Cancel() {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+  error_callback_.Run(BluetoothRemoteGattService::GATT_ERROR_FAILED);
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::OnSuccess() {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+
+  std::unique_ptr<device::BluetoothGattNotifySession> notify_session(
+      new BluetoothGattNotifySession(
+          characteristic_->weak_ptr_factory_.GetWeakPtr()));
+  characteristic_->notify_sessions_.insert(notify_session.get());
+  callback_.Run(std::move(notify_session));
+
+  if (!characteristic_->pending_notify_commands_.empty()) {
+    NotifySessionCommand* next =
+        characteristic_->pending_notify_commands_.front().get();
+    if (next->type_ == COMMAND_START) {
+      characteristic_->pending_notify_commands_.front()->OnSuccess();
+    } else {
+      characteristic_->pending_notify_commands_.front()->Execute();
+    }
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::OnError(
+    BluetoothRemoteGattService::GattErrorCode error) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+
+  error_callback_.Run(error);
+
+  if (!characteristic_->pending_notify_commands_.empty()) {
+    NotifySessionCommand* next =
+        characteristic_->pending_notify_commands_.front().get();
+    if (next->type_ == COMMAND_START) {

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

Do you do this for any other reason that optimizing?

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

Yes, if we have multiple StartNotifySession commands sitting in the front of the
queue, a single OnError or OnSuccess callback should resolve all of them.

+      characteristic_->pending_notify_commands_.front()->OnError(error);
+    } else {
+      characteristic_->pending_notify_commands_.front()->Execute();
+    }
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::StopNotifySession(
+    BluetoothGattNotifySession* session,
+    const base::Closure& callback) {
+  NotifySessionCommand* command =
+      new NotifySessionCommandStop(this, session, callback);
+  pending_notify_commands_.push(std::unique_ptr<NotifySessionCommand>(command));
+  if (pending_notify_commands_.size() == 1) {
+    command->Execute();
+  }
+}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::
+    NotifySessionCommandStop(BluetoothRemoteGattCharacteristic* characteristic,
+                             BluetoothGattNotifySession* session,
+                             const base::Closure& callback)
+    : NotifySessionCommand(characteristic, COMMAND_STOP),
+      session_(session),
+      callback_(callback) {}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::
+    ~NotifySessionCommandStop() {}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::Execute() {
+  std::set<BluetoothGattNotifySession*>::iterator session_iterator =
+      characteristic_->notify_sessions_.find(session_);
+
+  // If the session does not even belong to this characteristic, we return an
+  // error right away.
+  if (session_iterator == characteristic_->notify_sessions_.end()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::
+                       NotifySessionCommandStop::OnError,
+                   base::Unretained(this),
+                   device::BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  characteristic_->notify_sessions_.erase(session_);
+
+  // If there are more active sessions, then we return right away.
+  if (characteristic_->notify_sessions_.size() > 0) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&BluetoothRemoteGattCharacteristic::
+                                  NotifySessionCommandStop::OnSuccess,
+                              base::Unretained(this)));
+    return;
+  }
+
+  characteristic_->UnsubscribeFromNotifications(
+      base::Bind(&BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::
+                     OnSuccess,
+                 base::Unretained(this)),
+      base::Bind(
+          &BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::OnError,
+          base::Unretained(this)));
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::Cancel() {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+  callback_.Run();
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::OnSuccess() {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+
+  callback_.Run();
+
+  if (!characteristic_->pending_notify_commands_.empty()) {
+    characteristic_->pending_notify_commands_.front()->Execute();
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommandStop::OnError(
+    BluetoothRemoteGattService::GattErrorCode error) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(characteristic_->pending_notify_commands_.front());
+  DCHECK_EQ(command.get(), this);
+  characteristic_->pending_notify_commands_.pop();
+
+  callback_.Run();
+
+  if (!characteristic_->pending_notify_commands_.empty()) {
+    characteristic_->pending_notify_commands_.front()->Execute();
+  }
+}
+
 }  // namespace device