bluetooth: Subscribe to notifications

content/renderer/bluetooth/bluetooth_dispatcher.cc

Index: content/renderer/bluetooth/bluetooth_dispatcher.cc
diff --git a/content/renderer/bluetooth/bluetooth_dispatcher.cc b/content/renderer/bluetooth/bluetooth_dispatcher.cc
index 32323903c772a967d1725d25526dbd5b6a2e42ae..e414f17173ccaf549aaf10cf9bca6fef367d1d6b 100644
--- a/content/renderer/bluetooth/bluetooth_dispatcher.cc
+++ b/content/renderer/bluetooth/bluetooth_dispatcher.cc
@@ -14,6 +14,7 @@
 #include "third_party/WebKit/public/platform/WebPassOwnPtr.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h"
+#include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristic.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristicInit.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTService.h"
@@ -31,6 +32,8 @@ using blink::WebBluetoothScanFilter;
 using blink::WebRequestDeviceOptions;
 using blink::WebString;
 using blink::WebVector;
+using NotificationsRequestType =
+    content::BluetoothDispatcher::NotificationsRequestType;
 
 struct BluetoothPrimaryServiceRequest {
   BluetoothPrimaryServiceRequest(
@@ -62,6 +65,31 @@ struct BluetoothCharacteristicRequest {
   scoped_ptr<blink::WebBluetoothGetCharacteristicCallbacks> callbacks;
 };
 
+// Struct that holds a pending Start/StopNotifications request.
+struct BluetoothNotificationsRequest {
+  BluetoothNotificationsRequest(
+      const std::string characteristic_instance_id,
+      blink::WebBluetoothGATTCharacteristic* characteristic,
+      blink::WebBluetoothNotificationsCallbacks* callbacks,
+      NotificationsRequestType type)
+      : characteristic_instance_id(characteristic_instance_id),
+        characteristic(characteristic),
+        callbacks(callbacks),
+        type(type) {}
+  ~BluetoothNotificationsRequest() {}
+
+  const std::string characteristic_instance_id;
+  // The characteristic object is owned by the execution context on
+  // the blink side which can destroy the object at any point. Since the
+  // object implements ActiveDOMObject, the object calls Stop when is getting
+  // destroyed, which in turn calls characteristicObjectRemoved.
+  // characteristicObjectRemoved will null any pointers to the object
+  // and queue a stop notifications request if necessary.
+  blink::WebBluetoothGATTCharacteristic* characteristic;
+  scoped_ptr<blink::WebBluetoothNotificationsCallbacks> callbacks;
+  NotificationsRequestType type;
+};
+
 namespace content {
 
 namespace {
@@ -143,6 +171,12 @@ void BluetoothDispatcher::OnMessageReceived(const IPC::Message& msg) {
                       OnWriteValueSuccess);
   IPC_MESSAGE_HANDLER(BluetoothMsg_WriteCharacteristicValueError,
                       OnWriteValueError);
+  IPC_MESSAGE_HANDLER(BluetoothMsg_StartNotificationsSuccess,
+                      OnStartNotificationsSuccess)
+  IPC_MESSAGE_HANDLER(BluetoothMsg_StartNotificationsError,
+                      OnStartNotificationsError)
+  IPC_MESSAGE_HANDLER(BluetoothMsg_StopNotificationsSuccess,
+                      OnStopNotificationsSuccess)
   IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   DCHECK(handled) << "Unhandled message:" << msg.type();
@@ -225,10 +259,221 @@ void BluetoothDispatcher::writeValue(
       CurrentWorkerId(), request_id, characteristic_instance_id.utf8(), value));
 }
 
+void BluetoothDispatcher::startNotifications(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic,
+    blink::WebBluetoothNotificationsCallbacks* callbacks) {
+  int request_id = QueueNotificationRequest(characteristic_instance_id.utf8(),
+                                            characteristic, callbacks,
+                                            NotificationsRequestType::START);
+
+  // The Notification subscription's state can change after a request
+  // finishes. To avoid resolving with a soon-to-be-invalid state we queue
+  // requests.
+  if (PendingNotificationRequest(characteristic_instance_id.utf8())) {
+    return;
+  }
+
+  processStartNotificationsRequest(request_id);
+}
+
+void BluetoothDispatcher::stopNotifications(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic,
+    blink::WebBluetoothNotificationsCallbacks* callbacks) {
+  int request_id = QueueNotificationRequest(characteristic_instance_id.utf8(),
+                                            characteristic, callbacks,
+                                            NotificationsRequestType::STOP);
+  if (PendingNotificationRequest(characteristic_instance_id.utf8())) {
+    return;
+  }
+
+  processStopNotificationsRequest(request_id);
+}
+
+void BluetoothDispatcher::characteristicObjectRemoved(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic) {
+  // If there is a notification request pending then there are two posibilities:
+  //   1. The subscription is going to be active. If this is the case,
+  //      we null the characteristic object. startNotificationsSuccess will take
+  //      care of stopping the notifications.
+  //   2. The subscription is going to be inactive. If this is the case, we
+  //      don't need to to anything since the notification is stopped already.
+  if (PendingNotificationRequest(characteristic_instance_id.utf8())) {
+    for (IDMap<BluetoothNotificationsRequest, IDMapOwnPointer>::iterator iter(
+             &pending_notifications_requests_);
+         !iter.IsAtEnd(); iter.Advance()) {
+      iter.GetCurrentValue()->characteristic = nullptr;
+    }
+    return;
+  }
+
+  // If there are no notification requests pending then there are three
+  // possibilities after removing this characteristic:
+  //   1. The subscription was inactive already.
+  //   2. The subscription will become inactive.

[scheib, 2015/10/01 22:05:23]

Case 2 is only if another frame holds the subscription and releases, and can
only happen if !HasActiveNotificationSubscription, because there are no pending
notification requests and thus no action from this frame. I think just update
comments. Below I think #2 would go with the first if.

[ortuno, 2015/10/03 04:03:03]

Not sure I follow: Case 2 happens when this is the last frame holding the
notification.

[scheib, 2015/10/04 01:35:58]

Add detail to the comments somehow explaining case 2. Why will the subscription
become inactive? There's no pending request for it to become inactive. Oh, is it
because this object is the last object keeping it active and we must take action
to release it?

[ortuno, 2015/10/06 02:38:43]

Done.

+  //   3. The subscription will still be active.
+
+  if (!HasActiveNotificationSubscription(characteristic_instance_id.utf8())) {
+    return;
+  }
+
+  // For 2 and 3. We simply call processStopNotificationsRequest which will take
+  // care of stopping the notifications if the subscription becomes inactive.

[scheib, 2015/10/01 22:05:23]

Perhaps:
For 3 calling processStopNotificationsRequest ensures the notification
subscription is released.

[ortuno, 2015/10/03 04:03:03]

Done.

+  processStopNotificationsRequest(QueueNotificationRequest(
+      characteristic_instance_id.utf8(), characteristic,

[scheib, 2015/10/01 22:05:23]

characteristic should be nullptr

[ortuno, 2015/10/03 04:03:03]

We still need to remove the characteristic from the set of active notifications.

[scheib, 2015/10/04 01:35:58]

OK, yeah, this is complicated enough that's not obvious. Perhaps update that
comment above adding "is released for this characteristic object" to help hint
further? :(

[ortuno, 2015/10/06 02:38:43]

Done.

+      nullptr /* callbacks */, NotificationsRequestType::STOP));
+}
+
 void BluetoothDispatcher::WillStopCurrentWorkerThread() {
   delete this;
 }
 
+int BluetoothDispatcher::QueueNotificationRequest(
+    const std::string& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic,
+    blink::WebBluetoothNotificationsCallbacks* callbacks,
+    NotificationsRequestType type) {
+  int request_id =
+      pending_notifications_requests_.Add(new BluetoothNotificationsRequest(
+          characteristic_instance_id, characteristic, callbacks, type));
+  queued_notification_requests_[characteristic_instance_id].push(request_id);
+
+  return request_id;
+}
+
+void BluetoothDispatcher::ProcessQueuedNotificationRequests(int request_id) {
+  BluetoothNotificationsRequest* old_request =
+      pending_notifications_requests_.Lookup(request_id);
+
+  auto queue_iter = queued_notification_requests_.find(
+      old_request->characteristic_instance_id);
+
+  CHECK(queue_iter != queued_notification_requests_.end());
+
+  std::queue<int>& request_queue = queue_iter->second;
+
+  DCHECK(request_queue.front() == request_id);
+
+  // Remove old request and clean map if necessary.
+  request_queue.pop();
+  pending_notifications_requests_.Remove(request_id);
+  if (request_queue.empty()) {
+    queued_notification_requests_.erase(queue_iter);
+    return;
+  }
+
+  int next_request_id = request_queue.front();
+  BluetoothNotificationsRequest* next_request =
+      pending_notifications_requests_.Lookup(next_request_id);
+
+  switch (next_request->type) {
+    case NotificationsRequestType::START:
+      processStartNotificationsRequest(next_request_id);
+      return;
+    case NotificationsRequestType::STOP:
+      processStopNotificationsRequest(next_request_id);
+      return;
+  }
+}
+
+bool BluetoothDispatcher::PendingNotificationRequest(
+    const std::string& characteristic_instance_id) {
+  return queued_notification_requests_.find(characteristic_instance_id) !=
+             queued_notification_requests_.end() &&
+         (queued_notification_requests_[characteristic_instance_id].size() > 1);
+}
+
+bool BluetoothDispatcher::HasActiveNotificationSubscription(
+    const std::string& characteristic_instance_id) {
+  return ContainsKey(active_notification_subscriptions_,
+                     characteristic_instance_id);
+}
+
+void BluetoothDispatcher::AddToActiveNotificationSubscriptions(
+    const std::string& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic) {
+  active_notification_subscriptions_[characteristic_instance_id].insert(
+      characteristic);
+}
+
+bool BluetoothDispatcher::RemoveFromActiveNotificationSubscriptions(
+    const std::string& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristic* characteristic) {
+  auto active_map =
+      active_notification_subscriptions_.find(characteristic_instance_id);
+
+  if (active_map == active_notification_subscriptions_.end()) {
+    return false;
+  }
+
+  active_map->second.erase(characteristic);
+
+  if (active_map->second.empty()) {
+    active_notification_subscriptions_.erase(active_map);
+    DCHECK(!HasActiveNotificationSubscription(characteristic_instance_id));
+    return true;
+  }
+  return false;
+}
+
+void BluetoothDispatcher::processStartNotificationsRequest(int request_id) {
+  BluetoothNotificationsRequest* request =
+      pending_notifications_requests_.Lookup(request_id);
+  const std::string& characteristic_instance_id =
+      request->characteristic_instance_id;
+  blink::WebBluetoothGATTCharacteristic* characteristic =

[scheib, 2015/10/01 22:05:23]

characteristic may be nullptr due to characteristicObjectRemoved, in which case
we shouldn't Send a 'Start' right? For the 'Stop' we should. 

[ortuno, 2015/10/03 04:03:03]

That's tricky and seems like a question for Jeffrey: If the object gets
destroyed before we get to send a Start request should we even send the start
request?

[scheib, 2015/10/04 01:35:58]

Can wait for J if you like, but below on Start success and the characteristic
being nullptr we then send a stop right away. Seems pretty likely to me that we
shouldn't send the initial request here if the characteristic is already gone.
If another characteristic object needed it then it would exist and be in queue
to make it start.

[ortuno, 2015/10/06 02:38:43]

Chatted offline. We will send the request for now and open an issue on the spec.
https://github.com/WebBluetoothCG/web-bluetooth/issues/170

+      request->characteristic;
+  blink::WebBluetoothNotificationsCallbacks* callbacks =
+      request->callbacks.get();
+
+  // If an object is already subscribed to notifications from the characteristic
+  // no need to send an IPC again.
+  if (HasActiveNotificationSubscription(characteristic_instance_id)) {
+    AddToActiveNotificationSubscriptions(characteristic_instance_id,
+                                         characteristic);
+    callbacks->onSuccess();
+
+    ProcessQueuedNotificationRequests(request_id);
+    return;
+  }
+  Send(new BluetoothHostMsg_StartNotifications(CurrentWorkerId(), request_id,
+                                               characteristic_instance_id));
+}
+
+void BluetoothDispatcher::processStopNotificationsRequest(int request_id) {
+  // The Notification subscription's state can change after a request
+  // finishes. To avoid resolving with a soon-to-be-invalid state we queue
+  // requests.
+  BluetoothNotificationsRequest* request =
+      pending_notifications_requests_.Lookup(request_id);
+  const std::string& characteristic_instance_id =
+      request->characteristic_instance_id;
+  blink::WebBluetoothGATTCharacteristic* characteristic =
+      request->characteristic;
+  blink::WebBluetoothNotificationsCallbacks* callbacks =
+      request->callbacks.get();
+
+  // If removing turns the subscription inactive then stop.
+  if (RemoveFromActiveNotificationSubscriptions(characteristic_instance_id,
+                                                characteristic)) {

[scheib, 2015/10/01 22:05:23]

May be removing from here with characteristic == nullptr, when it was added with
a valid pointer. Perhaps simplest solution is to change
active_notification_subscriptions_ to std::map<std::string, int>

[ortuno, 2015/10/03 04:03:03]

That's why we pass the characteristic and not nullptr in
characteristicObjectRemoved

Perhaps simplest solution is to change active_notification_subscriptions_ to
std::map<std::string, int>

Would int be the number of references? That seems plausible. We would still need
another set to store the WebBluetoothCharacteristics though.

[scheib, 2015/10/04 01:35:58]

We do? I don't see WebBluetoothCharacteristics accessed from
active_notification_subscriptions_, only stored into it.

[ortuno, 2015/10/06 02:38:43]

It's not used yet. WebBluetoothCharacteristics is how we are going dispatch the
events to blink.

+    Send(new BluetoothHostMsg_StopNotifications(CurrentWorkerId(), request_id,
+                                                characteristic_instance_id));
+    return;
+  }
+  // An object is destroyed 1) during a start request or 2) after a start

[scheib, 2015/10/01 22:05:23]

Reform comment more like this for readability:

Possibly arrived here after two characteristic object destruction cases:
1) Characteristic is destroyed with a Start request outstanding: 
    Then OnStartNotificationsSuccess will queue a stop with 
    callback = nullptr and characteristic = nullptr which will
    eventually call processStopNotificationsRequest.
2) Characteristic is destroyed after Start request completes:
    The characteristic needs to be removed from
    active_notifications_subscriptions so
    characteristicObjectRemoved will call
    processStopNotification with callback = nullptr only.

    Comment didn't use "characteristicObjectRemoved"
    
After chat -- this can probably be simplified to much less text. Something like:
May be processing a request with nullptr callbacks due to
  1) Outstanding start request handed in OnStartNotificationsSuccess
      after characteristicObjectRemoved.
  2) Characteristic destroyed with no pending requests in
      characteristicObjectRemoved.

[ortuno, 2015/10/03 04:03:03]

Done.

[scheib, 2015/10/04 01:35:58]

I think you can delete the first 7 comment lines from this block.

[ortuno, 2015/10/06 02:38:43]

Done.

+  // request. If 1), then OnStartNotificationsSuccess will queue a stop with
+  // callback = nullptr and characteristic = nullptr which will eventually call
+  // processStopNotificationsRequest. If 2), characteristic needs to be removed
+  // from active_notifications_subscriptions so onCharacteristicObjectRemoved
+  // will call processStopNotification with callback = nullptr only.
+  if (callbacks != nullptr) {
+    callbacks->onSuccess();
+  }
+  ProcessQueuedNotificationRequests(request_id);
+}
+
 void BluetoothDispatcher::OnRequestDeviceSuccess(
     int thread_id,
     int request_id,
@@ -386,4 +631,78 @@ void BluetoothDispatcher::OnWriteValueError(int thread_id,
   pending_write_value_requests_.Remove(request_id);
 }
 
+void BluetoothDispatcher::OnStartNotificationsSuccess(int thread_id,
+                                                      int request_id) {
+  DCHECK(pending_notifications_requests_.Lookup(request_id)) << request_id;
+
+  BluetoothNotificationsRequest* request =
+      pending_notifications_requests_.Lookup(request_id);
+
+  DCHECK(queued_notification_requests_[request->characteristic_instance_id]
+             .front() == request_id);
+
+  // We only send an IPC for inactive notifications.
+  DCHECK(
+      !HasActiveNotificationSubscription(request->characteristic_instance_id));
+
+  AddToActiveNotificationSubscriptions(request->characteristic_instance_id,
+                                       request->characteristic);
+
+  // The object requesting the notification could have been destroyed
+  // while waiting for the subscription. characteristicRemoved
+  // nulls the characteristic when the corresponding js object gets destroyed.
+  // Since there could be other characteristics waiting to subscribe, we queue

[scheib, 2015/10/01 22:05:23]

I understand sending a Stop when the object is destroyed. And it seems that we
could do it here or in characteristicRemoved. I don't understand "Since there
could be other characteristics waiting to subscribe". Which others? How is
Stopping helping them? If no others just trim comment.

[ortuno, 2015/10/03 04:03:03]

// Frame 1
char.startNotifications()
// char gets destroyed
// Frame 2
char.startNotifications()

It's a small optimization that lets us keep the subscription alive by queuing
the Stop request after the start request from frame 2. By the time the Stop
request is processed frame 2 will hold the subscription.

[scheib, 2015/10/04 01:35:58]

Improve comments in code to address the uncertainty. It sound like the comment
wants to explain why a Stop is queued instead of being executed immediately. So,
comment along lines of:

// The object requesting the notification could have been destroyed while
waiting for the subscription. A STOP must be issued as the subscription is no
longer needed. The STOP is added to the end of the queue in case another START
exists in the queue from another characteristic object, which would result in
the subscription continuing.

[ortuno, 2015/10/06 02:38:43]

Done.

+  // a stop.
+  if (request->characteristic == nullptr) {
+    QueueNotificationRequest(
+        request->characteristic_instance_id, nullptr /* characteristic */,
+        nullptr /* callbacks */, NotificationsRequestType::STOP);
+  }
+
+  request->callbacks->onSuccess();
+
+  ProcessQueuedNotificationRequests(request_id);
+}
+
+void BluetoothDispatcher::OnStartNotificationsError(int thread_id,
+                                                    int request_id,
+                                                    WebBluetoothError error) {
+  DCHECK(pending_notifications_requests_.Lookup(request_id)) << request_id;
+
+  BluetoothNotificationsRequest* request =
+      pending_notifications_requests_.Lookup(request_id);
+
+  DCHECK(queued_notification_requests_[request->characteristic_instance_id]
+             .front() == request_id);
+
+  // We only send an IPC for inactive notifications.
+  DCHECK(
+      !HasActiveNotificationSubscription(request->characteristic_instance_id));
+
+  request->callbacks->onError(error);
+
+  ProcessQueuedNotificationRequests(request_id);
+}
+
+void BluetoothDispatcher::OnStopNotificationsSuccess(int thread_id,
+                                                     int request_id) {
+  DCHECK(pending_notifications_requests_.Lookup(request_id)) << request_id;
+
+  BluetoothNotificationsRequest* request =
+      pending_notifications_requests_.Lookup(request_id);
+
+  DCHECK(queued_notification_requests_[request->characteristic_instance_id]
+             .front() == request_id);
+
+  // We only send an IPC for inactive notifications.
+  DCHECK(
+      !HasActiveNotificationSubscription(request->characteristic_instance_id));
+
+  if (request->callbacks != nullptr) {
+    request->callbacks->onSuccess();
+  }
+
+  ProcessQueuedNotificationRequests(request_id);
+}
+
 }  // namespace content