bluetooth: Subscribe to notifications

content/renderer/bluetooth/bluetooth_dispatcher.cc

 // Copyright 2014 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.
 
 #include "content/renderer/bluetooth/bluetooth_dispatcher.h"
 
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/thread_task_runner_handle.h"
 #include "content/child/thread_safe_sender.h"
 #include "content/common/bluetooth/bluetooth_messages.h"
 #include "device/bluetooth/bluetooth_uuid.h"
 #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/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"
 
 using blink::WebBluetoothConnectGATTCallbacks;
 using blink::WebBluetoothDevice;
 using blink::WebBluetoothError;
 using blink::WebBluetoothGATTCharacteristic;
 using blink::WebBluetoothGATTRemoteServer;
 using blink::WebBluetoothGATTService;
@@ skipping 27 matching lines @@
       : service_instance_id(service_instance_id),
         characteristic_uuid(characteristic_uuid),
         callbacks(callbacks) {}
   ~BluetoothCharacteristicRequest() {}
 
   blink::WebString service_instance_id;
   blink::WebString characteristic_uuid;
   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 {
 
 base::LazyInstance<base::ThreadLocalPointer<void>>::Leaky g_dispatcher_tls =
     LAZY_INSTANCE_INITIALIZER;
 
 void* const kHasBeenDeleted = reinterpret_cast<void*>(0x1);
 
 int CurrentWorkerId() {
@@ skipping 61 matching lines @@
   IPC_MESSAGE_HANDLER(BluetoothMsg_GetCharacteristicError,
                       OnGetCharacteristicError);
   IPC_MESSAGE_HANDLER(BluetoothMsg_ReadCharacteristicValueSuccess,
                       OnReadValueSuccess);
   IPC_MESSAGE_HANDLER(BluetoothMsg_ReadCharacteristicValueError,
                       OnReadValueError);
   IPC_MESSAGE_HANDLER(BluetoothMsg_WriteCharacteristicValueSuccess,
                       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();
 }
 
 void BluetoothDispatcher::requestDevice(
     int frame_routing_id,
     const WebRequestDeviceOptions& options,
     blink::WebBluetoothRequestDeviceCallbacks* callbacks) {
   int request_id = pending_requests_.Add(callbacks);
@@ skipping 62 matching lines @@
 void BluetoothDispatcher::writeValue(
     const blink::WebString& characteristic_instance_id,
     const std::vector<uint8_t>& value,
     blink::WebBluetoothWriteValueCallbacks* callbacks) {
   int request_id = pending_write_value_requests_.Add(callbacks);
 
   Send(new BluetoothHostMsg_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,
     const BluetoothDevice& device) {
   DCHECK(pending_requests_.Lookup(request_id)) << request_id;
 
   WebVector<WebString> uuids(device.uuids.size());
   for (size_t i = 0; i < device.uuids.size(); ++i)
     uuids[i] = WebString::fromUTF8(device.uuids[i].c_str());
 
@@ skipping 137 matching lines @@
                                             int request_id,
                                             WebBluetoothError error) {
   DCHECK(pending_write_value_requests_.Lookup(request_id)) << request_id;
 
   pending_write_value_requests_.Lookup(request_id)
       ->onError(WebBluetoothError(error));
 
   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