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 1a4679e6d63291c6cb450c564ed1cd8f98f109ac..e9b256f53c63e9fd8c54aaee167f3ea469604386 100644
--- a/content/renderer/bluetooth/bluetooth_dispatcher.cc
+++ b/content/renderer/bluetooth/bluetooth_dispatcher.cc
@@ -15,6 +15,7 @@
 #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/WebBluetoothGATTCharacteristicDelegate.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTService.h"
 #include "third_party/WebKit/public/platform/modules/bluetooth/WebRequestDeviceOptions.h"
@@ -62,6 +63,21 @@ struct BluetoothCharacteristicRequest {
   scoped_ptr<blink::WebBluetoothGetCharacteristicCallbacks> callbacks;
 };
 
+struct BluetoothNotificationsRequest {
+  BluetoothNotificationsRequest(
+      blink::WebString characteristic_instance_id,
+      blink::WebBluetoothGATTCharacteristicDelegate* delegate,
+      blink::WebBluetoothNotificationsCallbacks* callbacks)
+      : characteristic_instance_id(characteristic_instance_id),
+        delegate(delegate),
+        callbacks(callbacks) {}
+  ~BluetoothNotificationsRequest() {}
+
+  blink::WebString characteristic_instance_id;
+  blink::WebBluetoothGATTCharacteristicDelegate* delegate;

[Jeffrey Yasskin, 2015/09/15 03:45:37]

Can you comment how we know |delegate| will stay alive long enough? (what object
owns it?)

[ortuno, 2015/09/15 21:38:19]

Well the execution context on the blink side owns the object and it can destroy
it at any point. But it does tell us when it's destroying the delegate so we can
remove any references to it. Added a note explaining that.

+  scoped_ptr<blink::WebBluetoothNotificationsCallbacks> callbacks;
+};
+
 namespace content {
 
 namespace {
@@ -143,6 +159,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 +247,126 @@ void BluetoothDispatcher::writeValue(
       CurrentWorkerId(), request_id, characteristic_instance_id.utf8(), value));
 }
 
+void BluetoothDispatcher::startNotifications(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristicDelegate* delegate,
+    blink::WebBluetoothNotificationsCallbacks* callbacks) {
+  // If an object is already subcribed to notifications from the characteristic,
+  // no need to subscribe to the dispatcher host again.
+  if (IsActiveCharacteristicNotification(characteristic_instance_id)) {
+    AddToActiveNotificationsMap(characteristic_instance_id, delegate);
+    callbacks->onSuccess();
+    return;
+  }
+
+  int request_id = pending_start_notifications_requests_.Add(
+      new BluetoothNotificationsRequest(characteristic_instance_id, delegate,
+                                        callbacks));
+
+  Send(new BluetoothHostMsg_StartNotifications(

[Jeffrey Yasskin, 2015/09/15 03:45:37]

Since you're creating the active_notifications entry when this IPC comes back,
make sure that if a concurrent call to startNotifications() comes in before this
IPC returns, it doesn't cause a second IPC to be sent. You'll probably need a
queue of callbacks that gets flushed when the IPC returns.

[ortuno, 2015/09/15 21:38:19]

That seems like an unnecessary optimization. Sending another IPC doesn't really
cost us much and keeps the code simple.

+      CurrentWorkerId(), request_id, characteristic_instance_id.utf8()));
+}
+
+void BluetoothDispatcher::stopNotifications(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristicDelegate* delegate,
+    blink::WebBluetoothNotificationsCallbacks* callbacks) {
+  // If this is not an active characteristic then no need to unsubscribe.
+  if (!IsActiveCharacteristicNotification(characteristic_instance_id)) {

[Jeffrey Yasskin, 2015/09/15 03:45:37]

I think you don't need this check. Removing the element will be a no-op if the
notification wasn't active.

[ortuno, 2015/09/15 21:38:19]

Reworked this part. Calling stopNotifications twice in a row would result in one
IPC being sent and the second call to stopNotifications succeeding immediately.

+    callbacks->onSuccess();
+    return;
+  }
+
+  RemoveFromActiveNotificationsMap(characteristic_instance_id, delegate);
+
+  // If the characteristic is still active no need to unsubscribe.
+  if (IsActiveCharacteristicNotification(characteristic_instance_id)) {
+    callbacks->onSuccess();
+    return;
+  }
+
+  int request_id = pending_stop_notifications_requests_.Add(
+      new BluetoothNotificationsRequest(characteristic_instance_id, delegate,
+                                        callbacks));
+
+  Send(new BluetoothHostMsg_StopNotifications(
+      CurrentWorkerId(), request_id, characteristic_instance_id.utf8()));
+}
+
+void BluetoothDispatcher::characteristicDelegateRemoved(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristicDelegate* delegate) {
+  RemoveFromActiveNotificationsMap(characteristic_instance_id, delegate);
+
+  // If the characteristic is still active no need to unsubscribe.
+  if (IsActiveCharacteristicNotification(characteristic_instance_id)) {
+    return;
+  }
+
+  // There could be a request pending on a delegate so we need to check
+  // the requests map. We ignore the stop request since we don't use
+  // the delegate in those.
+  for (IDMap<BluetoothNotificationsRequest, IDMapOwnPointer>::iterator iter(
+           &pending_start_notifications_requests_);
+       !iter.IsAtEnd(); iter.Advance()) {
+    iter.GetCurrentValue()->delegate = nullptr;
+  }
+}
+
 void BluetoothDispatcher::WillStopCurrentWorkerThread() {
   delete this;
 }
 
+bool BluetoothDispatcher::IsActiveCharacteristicNotification(
+    const blink::WebString& characteristic_instance_id) {
+  auto char_iter = active_characteristic_notifications.find(
+      characteristic_instance_id.utf8());
+
+  // If this is the first object that is subscribing to notifications then
+  // the characteristic is not active.
+  if (char_iter == active_characteristic_notifications.end()) {
+    return false;
+  }
+
+  // If no object is subscribed to notifications then the characteristic
+  // is not active.
+  if (char_iter->second.size() == 0) {

[Jeffrey Yasskin, 2015/09/15 03:45:37]

We should make sure that we remove elements from the map when their set gets
empty, rather than leaving empty mappings around using up memory.

[ortuno, 2015/09/15 21:38:20]

Done.

+    return false;
+  }
+  return true;
+}
+
+void BluetoothDispatcher::AddToActiveNotificationsMap(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristicDelegate* delegate) {
+  auto char_iter = active_characteristic_notifications.find(

[Jeffrey Yasskin, 2015/09/15 03:45:37]

This is a good place to use operator[]:

std::set<blink::WebBluetoothGATTCharacteristicDelegate*>& active_notifications =
  active_characteristic_notifications[characteristic_instance_id.utf8()];

bool should_register = active_notifications.empty();

active_notifications.insert(delegate);

return should_register;  // Avoid needing a double-lookup in the caller.

[ortuno, 2015/09/15 21:38:20]

I don't like adding the delegates to the map before the ipc returns. Makes it
trickier to handle concurrent calls to start/stop.

+      characteristic_instance_id.utf8());
+
+  // First object that subscribes to notifications for the characteristic.
+  if (char_iter == active_characteristic_notifications.end()) {
+    std::set<blink::WebBluetoothGATTCharacteristicDelegate*> delegates;
+    delegates.insert(delegate);
+    active_characteristic_notifications.insert(
+        std::make_pair(characteristic_instance_id.utf8(), delegates));
+    return;
+  }
+  // Otherwise add to already existing set.
+  char_iter->second.insert(delegate);
+}
+
+void BluetoothDispatcher::RemoveFromActiveNotificationsMap(
+    const blink::WebString& characteristic_instance_id,
+    blink::WebBluetoothGATTCharacteristicDelegate* delegate) {
+  auto char_iter = active_characteristic_notifications.find(
+      characteristic_instance_id.utf8());
+
+  if (char_iter == active_characteristic_notifications.end()) {
+    return;
+  }
+
+  char_iter->second.erase(delegate);

[Jeffrey Yasskin, 2015/09/15 03:45:37]

If the set became empty, remove it from the map here.

[ortuno, 2015/09/15 21:38:20]

Done.

+}
+
 void BluetoothDispatcher::OnRequestDeviceSuccess(
     int thread_id,
     int request_id,
@@ -386,4 +524,51 @@ void BluetoothDispatcher::OnWriteValueError(int thread_id,
   pending_write_value_requests_.Remove(request_id);
 }
 
+void BluetoothDispatcher::OnStartNotificationsSuccess(int thread_id,
+                                                      int request_id) {
+  DCHECK(pending_start_notifications_requests_.Lookup(request_id))
+      << request_id;
+
+  BluetoothNotificationsRequest* request =
+      pending_start_notifications_requests_.Lookup(request_id);
+
+  // The object requesting the notification could have been destroyed
+  // while waiting for the subscription.
+  if (request->delegate != nullptr) {

[Jeffrey Yasskin, 2015/09/15 03:45:37]

Could you comment that characteristicDelegateRemoved() nulls this out?

[ortuno, 2015/09/15 21:38:19]

Done.

+    AddToActiveNotificationsMap(request->characteristic_instance_id,
+                                request->delegate);
+  }
+  request->callbacks->onSuccess();
+
+  pending_start_notifications_requests_.Remove(request_id);
+}
+
+void BluetoothDispatcher::OnStartNotificationsError(int thread_id,
+                                                    int request_id,
+                                                    WebBluetoothError error) {
+  DCHECK(pending_start_notifications_requests_.Lookup(request_id))
+      << request_id;
+
+  pending_start_notifications_requests_.Lookup(request_id)
+      ->callbacks->onError(error);
+
+  pending_start_notifications_requests_.Remove(request_id);
+}
+
+void BluetoothDispatcher::OnStopNotificationsSuccess(int thread_id,
+                                                     int request_id) {
+  DCHECK(pending_stop_notifications_requests_.Lookup(request_id)) << request_id;
+
+  BluetoothNotificationsRequest* request =
+      pending_stop_notifications_requests_.Lookup(request_id);
+
+  RemoveFromActiveNotificationsMap(request->characteristic_instance_id,

[Jeffrey Yasskin, 2015/09/15 03:45:37]

You already called this before sending the stop IPC.

[ortuno, 2015/09/15 21:38:20]

No longer calling this from stopNotifications.

+                                   request->delegate);
+
+  pending_stop_notifications_requests_.Lookup(request_id)
+      ->callbacks->onSuccess();
+
+  pending_stop_notifications_requests_.Remove(request_id);
+}
+
 }  // namespace content