Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

device/bluetooth/bluetooth_remote_gatt_characteristic.h

 // Copyright 2016 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.
 
 #ifndef DEVICE_BLUETOOTH_BLUETOOTH_REMOTE_GATT_CHARACTERISTIC_H_
 #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 {
 
 class BluetoothGattNotifySession;
 class BluetoothRemoteGattDescriptor;
 class BluetoothRemoteGattService;
 
 // BluetoothRemoteGattCharacteristic represents a remote GATT characteristic.
 // This class is used to represent GATT characteristics that belong to a service
@@ skipping 18 matching lines @@
       NotifySessionCallback;
 
   // Returns the value of the characteristic. For remote characteristics, this
   // is the most recently cached value. For local characteristics, this is the
   // most recently updated value or the value retrieved from the delegate.
   virtual const std::vector<uint8_t>& GetValue() const = 0;
 
   // 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()
       const = 0;
 
   // Returns the GATT characteristic descriptor with identifier |identifier| if
   // it belongs to this GATT characteristic.
   virtual BluetoothRemoteGattDescriptor* GetDescriptor(
       const std::string& identifier) const = 0;
 
   // Returns the GATT characteristic descriptors that match |uuid|. There may be
   // 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
   // |error_callback| is called for failures.
   virtual void ReadRemoteCharacteristic(
       const ValueCallback& callback,
       const ErrorCallback& error_callback) = 0;
 
   // Sends a write request to a remote characteristic, to modify the
   // characteristic's value with the new value |new_value|. |callback| is
   // called to signal success and |error_callback| for failures. This method
   // only applies to remote characteristics and will fail for those that are
   // locally hosted.
   virtual void WriteRemoteCharacteristic(
       const std::vector<uint8_t>& new_value,
       const base::Closure& callback,
       const ErrorCallback& error_callback) = 0;
 
  protected:
   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);
 };
 
 }  // namespace device
 
 #endif  // DEVICE_BLUETOOTH_BLUETOOTH_REMOTE_GATT_CHARACTERISTIC_H_