Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

device/bluetooth/bluetooth_remote_gatt_characteristic.h

Index: device/bluetooth/bluetooth_remote_gatt_characteristic.h
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic.h b/device/bluetooth/bluetooth_remote_gatt_characteristic.h
index d8f9ff6dbdb8d0d9d0340d6bc37d60558e6fa8ab..66a1b017b34a408621c58d70373bff3c403c143e 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic.h
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic.h
@@ -6,14 +6,19 @@
 #define DEVICE_BLUETOOTH_BLUETOOTH_REMOTE_GATT_CHARACTERISTIC_H_
 
 #include <stdint.h>
+
+#include <queue>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "device/bluetooth/bluetooth_export.h"
 #include "device/bluetooth/bluetooth_gatt_characteristic.h"
+#include "device/bluetooth/bluetooth_remote_gatt_service.h"
 #include "device/bluetooth/bluetooth_uuid.h"
 
 namespace device {
@@ -52,10 +57,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothRemoteGattCharacteristic
   // Returns a pointer to the GATT service this characteristic belongs to.
   virtual BluetoothRemoteGattService* GetService() const = 0;
 
-  // Returns whether or not this characteristic is currently sending value
-  // updates in the form of a notification or indication.
-  virtual bool IsNotifying() const = 0;
-
   // Returns the list of GATT characteristic descriptors that provide more
   // information about this characteristic.
   virtual std::vector<BluetoothRemoteGattDescriptor*> GetDescriptors()
@@ -70,20 +71,36 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothRemoteGattCharacteristic
   // multiple, as illustrated by Core Bluetooth Specification [V4.2 Vol 3 Part G
   // 3.3.3.5 Characteristic Presentation Format].
   std::vector<BluetoothRemoteGattDescriptor*> GetDescriptorsByUUID(
-      const BluetoothUUID& uuid);
+      const BluetoothUUID& uuid) const;
+
+  base::WeakPtr<device::BluetoothRemoteGattCharacteristic> GetWeakPtr();
+
+  // Returns whether or not this characteristic is currently sending value
+  // updates in the form of a notification or indication.
+  virtual bool IsNotifying() const;

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

hmm this should really be private. I can't think of any use cases for it. There
are two uses in the codebase: One of them is using it completely wrong and the
other one is working around a bug that was fixed... OK to leave public for now
but can you add the following:

// TODO(crbug.com/632476): Make private since there are no uses for this
function.

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

Even though there are no good uses of this function at the moment, there could
be in the future. I feel that this function is a potentially useful and
perfectly reasonable part of this class' public api.

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

I think this makes the API a bit confusing and doesn't really provide any useful
information to clients i.e. why would a client care if there are any other
clients that have notify sessions? Anyway I don't feel too strongly about it,
but I would feel better if the documentation is improved a bit. I mainly want to
avoid cases like this one: http://crrev.com/2189183002

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

I've updated the documentation.

 
   // Starts a notify session for the remote characteristic, if it supports
   // notifications/indications. On success, the characteristic starts sending
   // value notifications and |callback| is called with a session object whose
   // ownership belongs to the caller. |error_callback| is called on errors.

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

I think we should document this more. Please explain:

- How notifications are delivered e.g. CharacteristicValueChanged is called
when...
- How to stop notifications e.g. calling session->Stop or destroying the
session.

Anything else that you think might be relevant for someone that wants to use
this API.

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

Done.

   //
-  // Writes to the Client Characteristic Configuration descriptor to enable
-  // notifications/indications. Core Bluetooth Specification [V4.2 Vol 3 Part G
-  // Section 3.3.1.1. Characteristic Properties] requires this descriptor to be
-  // present when notifications/indications are supported. If the descriptor is
-  // not present |error_callback| will be run.
+  // This method handles all logic regarding multiple sessions so that
+  // specific platform implementations of the remote characteristic class
+  // do not have to. Rather than overriding this method, it is recommended
+  // to override the SubscribeToNotifications method below.
   virtual void StartNotifySession(const NotifySessionCallback& callback,
-                                  const ErrorCallback& error_callback) = 0;
+                                  const ErrorCallback& error_callback);
+
+  // Stops an active notify session for the remote characteristic. On success,
+  // the characteristic removes this session from the list of active sessions.
+  // If there are no more active sessions, notifications/indications are
+  // turned off.
+  //
+  // It is recommended for platform implementations of the remote charactiristic
+  // class to override UnsubscribeFromNotifications below, instead of overriding
+  // this method.
+  virtual void StopNotifySession(BluetoothGattNotifySession* session,

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

I don't think we want to expose another way of stopping sessions. Make this
private and have BluetoothNotifySession be a friend of this class.

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

Done.

+                                 const base::Closure& callback);
 
   // Sends a read request to a remote characteristic to read its value.
   // |callback| is called to return the read value on success and
@@ -106,7 +123,82 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothRemoteGattCharacteristic
   BluetoothRemoteGattCharacteristic();
   ~BluetoothRemoteGattCharacteristic() override;
 
+  // Writes to the Client Characteristic Configuration descriptor to enable

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

Also could you explain the relationship between these two methods and
Start/StopNotifySessions? No need to go into too much detail, just saying that
these two methods are meant to be called by Start/StopNotifySessions might be
enough.

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

Done.

+  // notifications/indications. Core Bluetooth Specification [V4.2 Vol 3 Part G
+  // Section 3.3.1.1. Characteristic Properties] requires this descriptor to be
+  // present when notifications/indications are supported. If the descriptor is
+  // not present |error_callback| will be run.

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

We probably want this behavior, failing on no descriptor, to be cross platform.
Do you think we could include this in StartNotifySession? Or maybe implement
this here and have other implementations override SubscribeToNotificationsImpl?

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

I have moved this behavior to
BluetoothRemoteGattCharacteristic::NotifySessionCommandStart::Execute().

+  virtual void SubscribeToNotifications(
+      const base::Closure& callback,
+      const ErrorCallback& error_callback) = 0;
+
+  // Writes to the Client Characteristic Configuration descriptor to disable
+  // notifications/indications. Core Bluetooth Specification [V4.2 Vol 3 Part G
+  // Section 3.3.1.1. Characteristic Properties] requires this descriptor to be
+  // present when notifications/indications are supported. If the descriptor is
+  // not present |error_callback| will be run.
+  virtual void UnsubscribeFromNotifications(
+      const base::Closure& callback,
+      const ErrorCallback& error_callback) = 0;
+
  private:
+  class NotifySessionCommand {
+   public:
+    enum Type { COMMAND_START, COMMAND_STOP };
+
+    BluetoothRemoteGattCharacteristic* characteristic_;
+    Type type_;
+
+    NotifySessionCommand(BluetoothRemoteGattCharacteristic* characteristic,
+                         Type type);
+    virtual ~NotifySessionCommand();
+
+    virtual void Execute() = 0;
+    virtual void Cancel() = 0;
+    virtual void OnSuccess() = 0;
+    virtual void OnError(BluetoothRemoteGattService::GattErrorCode error) = 0;
+  };
+
+  class NotifySessionCommandStart : public NotifySessionCommand {
+   public:
+    NotifySessionCommandStart(BluetoothRemoteGattCharacteristic* characteristic,
+                              const NotifySessionCallback& callback,
+                              const ErrorCallback& error_callback);
+    ~NotifySessionCommandStart() override;
+
+    NotifySessionCallback callback_;
+    ErrorCallback error_callback_;
+
+    void Execute() override;
+    void Cancel() override;
+    void OnSuccess() override;
+    void OnError(BluetoothRemoteGattService::GattErrorCode error) override;
+  };
+
+  class NotifySessionCommandStop : public NotifySessionCommand {
+   public:
+    NotifySessionCommandStop(BluetoothRemoteGattCharacteristic* characteristic,
+                             BluetoothGattNotifySession* session,
+                             const base::Closure& callback);
+    ~NotifySessionCommandStop() override;
+
+    BluetoothGattNotifySession* session_;
+    base::Closure callback_;
+
+    void Execute() override;
+    void Cancel() override;
+    void OnSuccess() override;
+    void OnError(BluetoothRemoteGattService::GattErrorCode error) override;
+  };
+
+  // Pending StartNotifySession / StopNotifySession calls.
+  std::queue<std::unique_ptr<NotifySessionCommand>> pending_notify_commands_;
+
+  // Set of active notify sessions.
+  std::set<BluetoothGattNotifySession*> notify_sessions_;
+
+  base::WeakPtrFactory<BluetoothRemoteGattCharacteristic> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(BluetoothRemoteGattCharacteristic);
 };