Refactor the BluetoothDispatcherHost to store requestDevice sessions

content/browser/bluetooth/bluetooth_dispatcher_host.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.
 
 // NETWORK_ERROR Note:
 // When a device can't be found in the BluetoothAdapter, that generally
 // indicates that it's gone out of range. We reject with a NetworkError in that
 // case.
 // https://webbluetoothchrome.github.io/web-bluetooth/#dom-bluetoothdevice-connectgatt
 
@@ skipping 168 matching lines @@
   current_delay_time_ = kTestingDelayTime;
   set_adapter(mock_adapter.Pass());
 }
 
 BluetoothDispatcherHost::~BluetoothDispatcherHost() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Clear adapter, releasing observer references.
   set_adapter(scoped_refptr<device::BluetoothAdapter>());
 }
 
-struct BluetoothDispatcherHost::DiscoverySessionOptions {
-  DiscoverySessionOptions(const std::vector<BluetoothScanFilter>& filters,
-                          const std::vector<BluetoothUUID>& optional_services)
+struct BluetoothDispatcherHost::RequestDeviceSession {
+  RequestDeviceSession(const std::vector<BluetoothScanFilter>& filters,
+                       const std::vector<BluetoothUUID>& optional_services)
       : filters(filters), optional_services(optional_services) {}
 
   std::vector<BluetoothScanFilter> filters;
   std::vector<BluetoothUUID> optional_services;
 };
 
 void BluetoothDispatcherHost::set_adapter(
     scoped_refptr<device::BluetoothAdapter> adapter) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (adapter_.get())
@@ skipping 20 matching lines @@
 
 void BluetoothDispatcherHost::OnRequestDevice(
     int thread_id,
     int request_id,
     const std::vector<BluetoothScanFilter>& filters,
     const std::vector<BluetoothUUID>& optional_services) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // TODO(scheib): Device selection UI: crbug.com/436280
   // TODO(scheib): Utilize BluetoothAdapter::Observer::DeviceAdded/Removed.
   if (adapter_.get()) {
+    if (!request_device_sessions_
+             .insert(std::make_pair(
+                 std::make_pair(thread_id, request_id),

[ortuno, 2015/07/15 22:39:12]

Would an iframe and it's parent use the same web_bluetooth_impl? I'm worried
that since thread_id is always 0 you might have some collisions with request_id.

Maybe add tests to make sure you never get coalitions?

[Jeffrey Yasskin, 2015/07/17 01:07:23]

They'll use the same BluetoothDispatcher (since it's looked up by thread), and
it's BluetoothDispatcher that allocates the request_id.
Sure: https://codereview.chromium.org/1227863007.

+                 RequestDeviceSession(filters, optional_services)))
+             .second) {
+      LOG(ERROR) << "2 requestDevice() calls with the same thread_id ("
+                 << thread_id << ") and request_id (" << request_id
+                 << ") shouldn't arrive at the same BluetoothDispatcherHost.";
+      bad_message::ReceivedBadMessage(
+          this, bad_message::BDH_DUPLICATE_REQUEST_DEVICE_ID);
+    }
     adapter_->StartDiscoverySessionWithFilter(
         ComputeScanFilter(filters),
         base::Bind(&BluetoothDispatcherHost::OnDiscoverySessionStarted,
-                   weak_ptr_factory_.GetWeakPtr(), thread_id, request_id,
-                   base::Passed(make_scoped_ptr(new DiscoverySessionOptions(
-                       filters, optional_services)))),
+                   weak_ptr_factory_.GetWeakPtr(), thread_id, request_id),
         base::Bind(&BluetoothDispatcherHost::OnDiscoverySessionStartedError,
                    weak_ptr_factory_.GetWeakPtr(), thread_id, request_id));
   } else {
     DLOG(WARNING) << "No BluetoothAdapter. Can't serve requestDevice.";
     Send(new BluetoothMsg_RequestDeviceError(
         thread_id, request_id, WebBluetoothError::NoBluetoothAdapter));
   }
   return;
 }
 
@@ skipping 217 matching lines @@
   characteristic->WriteRemoteCharacteristic(
       value, base::Bind(&BluetoothDispatcherHost::OnWriteValueSuccess,
                         weak_ptr_factory_.GetWeakPtr(), thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnWriteValueFailed,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnDiscoverySessionStarted(
     int thread_id,
     int request_id,
-    scoped_ptr<DiscoverySessionOptions> options,
     scoped_ptr<device::BluetoothDiscoverySession> discovery_session) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BrowserThread::PostDelayedTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&BluetoothDispatcherHost::StopDiscoverySession,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id,
-                 base::Passed(&options), base::Passed(&discovery_session)),
+                 base::Passed(&discovery_session)),
       base::TimeDelta::FromSeconds(current_delay_time_));
 }
 
 void BluetoothDispatcherHost::OnDiscoverySessionStartedError(int thread_id,
                                                              int request_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DLOG(WARNING) << "BluetoothDispatcherHost::OnDiscoverySessionStartedError";
   Send(new BluetoothMsg_RequestDeviceError(
       thread_id, request_id, WebBluetoothError::DiscoverySessionStartFailed));
+  request_device_sessions_.erase(std::make_pair(thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::StopDiscoverySession(
     int thread_id,
     int request_id,
-    scoped_ptr<DiscoverySessionOptions> options,
     scoped_ptr<device::BluetoothDiscoverySession> discovery_session) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   discovery_session->Stop(
       base::Bind(&BluetoothDispatcherHost::OnDiscoverySessionStopped,
-                 weak_ptr_factory_.GetWeakPtr(), thread_id, request_id,
-                 base::Passed(&options)),
+                 weak_ptr_factory_.GetWeakPtr(), thread_id, request_id),
       base::Bind(&BluetoothDispatcherHost::OnDiscoverySessionStoppedError,
                  weak_ptr_factory_.GetWeakPtr(), thread_id, request_id));
 }
 
-void BluetoothDispatcherHost::OnDiscoverySessionStopped(
-    int thread_id,
-    int request_id,
-    scoped_ptr<DiscoverySessionOptions> options) {
+void BluetoothDispatcherHost::OnDiscoverySessionStopped(int thread_id,
+                                                        int request_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto session =
+      request_device_sessions_.find(std::make_pair(thread_id, request_id));

[ortuno, 2015/07/15 22:39:12]

We use thread_id + request_id everywhere. What do you think about passing them
together through IPC?

[Jeffrey Yasskin, 2015/07/17 01:07:23]

It'd make sense to do that, but I'd rather do it after this patch. We'll also
have to double-check how to keep
BluetoothMessageFilter::GetWorkerThreadIdForMessage working.

+  CHECK(session != request_device_sessions_.end());
   BluetoothAdapter::DeviceList devices = adapter_->GetDevices();
   for (device::BluetoothDevice* device : devices) {
-    if (MatchesFilters(*device, options->filters)) {
+    if (MatchesFilters(*device, session->second.filters)) {
       content::BluetoothDevice device_ipc(
           device->GetAddress(),         // instance_id
           device->GetName(),            // name
           device->GetBluetoothClass(),  // device_class
           device->GetVendorIDSource(),  // vendor_id_source
           device->GetVendorID(),        // vendor_id
           device->GetProductID(),       // product_id
           device->GetDeviceID(),        // product_version
           device->IsPaired(),           // paired
           content::BluetoothDevice::UUIDsFromBluetoothUUIDs(
               device->GetUUIDs()));  // uuids
       Send(new BluetoothMsg_RequestDeviceSuccess(thread_id, request_id,
                                                  device_ipc));
+      request_device_sessions_.erase(session);
       return;
     }
   }
   Send(new BluetoothMsg_RequestDeviceError(thread_id, request_id,
                                            WebBluetoothError::NoDevicesFound));
+  request_device_sessions_.erase(session);
 }
 
 void BluetoothDispatcherHost::OnDiscoverySessionStoppedError(int thread_id,
                                                              int request_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DLOG(WARNING) << "BluetoothDispatcherHost::OnDiscoverySessionStoppedError";
   Send(new BluetoothMsg_RequestDeviceError(
       thread_id, request_id, WebBluetoothError::DiscoverySessionStopFailed));
+  request_device_sessions_.erase(std::make_pair(thread_id, request_id));
 }
 
 void BluetoothDispatcherHost::OnGATTConnectionCreated(
     int thread_id,
     int request_id,
     const std::string& device_instance_id,
     scoped_ptr<device::BluetoothGattConnection> connection) {
   // TODO(ortuno): Save the BluetoothGattConnection so we can disconnect
   // from it.
   Send(new BluetoothMsg_ConnectGATTSuccess(thread_id, request_id,
@@ skipping 69 matching lines @@
 
 void BluetoothDispatcherHost::OnWriteValueFailed(
     int thread_id,
     int request_id,
     device::BluetoothGattService::GattErrorCode error_code) {
   Send(new BluetoothMsg_WriteCharacteristicValueError(
       thread_id, request_id, TranslateGATTError(error_code)));
 }
 
 }  // namespace content