Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

content/browser/bluetooth/web_bluetooth_service_impl.cc

 // 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.
 
 // ID Not In Map Note: A service, characteristic, or descriptor ID not in the
 // corresponding WebBluetoothServiceImpl map [service_id_to_device_address_,
 // characteristic_id_to_service_id_, descriptor_id_to_characteristic_id_]
 // implies a hostile renderer because a renderer obtains the corresponding ID
 // from this class and it will be added to the map at that time.
 
 #include "content/browser/bluetooth/web_bluetooth_service_impl.h"
 
 #include <algorithm>
 
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/bluetooth/bluetooth_blocklist.h"
 #include "content/browser/bluetooth/bluetooth_device_chooser_controller.h"
 #include "content/browser/bluetooth/bluetooth_metrics.h"
 #include "content/browser/bluetooth/frame_connected_bluetooth_devices.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/common/bluetooth/web_bluetooth_device_id.h"
 #include "content/public/browser/browser_context.h"
@@ skipping 115 matching lines @@
     return blink::mojom::WebBluetoothResult::DEVICE_NO_LONGER_IN_RANGE;
   }
 
   device::BluetoothDevice* device = nullptr;
   device::BluetoothRemoteGattService* service = nullptr;
   device::BluetoothRemoteGattCharacteristic* characteristic = nullptr;
   device::BluetoothRemoteGattDescriptor* descriptor = nullptr;
   CacheQueryOutcome outcome;
 };
 
+struct GATTNotifySessionAndCharacteristicClient {
+  GATTNotifySessionAndCharacteristicClient(
+      std::unique_ptr<device::BluetoothGattNotifySession> session,
+      blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr client)
+      : gatt_notify_session(std::move(session)),
+        characteristic_client(std::move(client)) {}
+
+  std::unique_ptr<device::BluetoothGattNotifySession> gatt_notify_session;
+  blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr
+      characteristic_client;
+};
+
 WebBluetoothServiceImpl::WebBluetoothServiceImpl(
     RenderFrameHost* render_frame_host,
     blink::mojom::WebBluetoothServiceRequest request)
     : WebContentsObserver(WebContents::FromRenderFrameHost(render_frame_host)),
       connected_devices_(new FrameConnectedBluetoothDevices(render_frame_host)),
       render_frame_host_(render_frame_host),
       binding_(this, std::move(request)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   CHECK(web_contents());
@@ skipping 45 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (device_chooser_controller_.get()) {
     device_chooser_controller_->AddFilteredDevice(*device);
   }
 
   if (!device->IsGattConnected()) {
     base::Optional<WebBluetoothDeviceId> device_id =
         connected_devices_->CloseConnectionToDeviceWithAddress(
             device->GetAddress());
-    if (device_id && client_) {
-      client_->GattServerDisconnected(device_id.value());
-    }
   }
 }
 
 void WebBluetoothServiceImpl::GattServicesDiscovered(
     device::BluetoothAdapter* adapter,
     device::BluetoothDevice* device) {
   if (device_chooser_controller_.get()) {
     device_chooser_controller_->AddFilteredDevice(*device);
   }
 
@@ skipping 36 matching lines @@
           base::Bind(&WebBluetoothServiceImpl::NotifyCharacteristicValueChanged,
                      weak_ptr_factory_.GetWeakPtr(),
                      characteristic->GetIdentifier(), value))) {
     LOG(WARNING) << "No TaskRunner.";
   }
 }
 
 void WebBluetoothServiceImpl::NotifyCharacteristicValueChanged(
     const std::string& characteristic_instance_id,
     const std::vector<uint8_t>& value) {
-  if (client_) {
-    client_->RemoteCharacteristicValueChanged(characteristic_instance_id,
-                                              value);
+  auto iter =
+      characteristic_id_to_notify_session_.find(characteristic_instance_id);
+  if (iter != characteristic_id_to_notify_session_.end()) {
+    iter->second->characteristic_client->RemoteCharacteristicValueChanged(
+        value);
   }
 }
 
-void WebBluetoothServiceImpl::SetClient(
-    blink::mojom::WebBluetoothServiceClientAssociatedPtrInfo client) {
-  DCHECK(!client_.get());
-  client_.Bind(std::move(client));
-}
-
 void WebBluetoothServiceImpl::RequestDevice(
     blink::mojom::WebBluetoothRequestDeviceOptionsPtr options,
     const RequestDeviceCallback& callback) {
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::REQUEST_DEVICE);
   RecordRequestDeviceOptions(options);
 
   if (!GetAdapter()) {
     if (BluetoothAdapterFactoryWrapper::Get().IsLowEnergyAvailable()) {
       BluetoothAdapterFactoryWrapper::Get().AcquireAdapter(
           this, base::Bind(&WebBluetoothServiceImpl::RequestDeviceImpl,
                            weak_ptr_factory_.GetWeakPtr(),
                            base::Passed(std::move(options)), callback));
       return;
     }
     RecordRequestDeviceOutcome(
         UMARequestDeviceOutcome::BLUETOOTH_LOW_ENERGY_NOT_AVAILABLE);
     callback.Run(
         blink::mojom::WebBluetoothResult::BLUETOOTH_LOW_ENERGY_NOT_AVAILABLE,
         nullptr /* device */);
     return;
   }
   RequestDeviceImpl(std::move(options), callback, GetAdapter());
 }
 
 void WebBluetoothServiceImpl::RemoteServerConnect(
     const WebBluetoothDeviceId& device_id,
+    blink::mojom::WebBluetoothServerClientAssociatedPtrInfo client,
     const RemoteServerConnectCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::CONNECT_GATT);
 
   const CacheQueryResult query_result = QueryCacheForDevice(device_id);
 
   if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
     RecordConnectGATTOutcome(query_result.outcome);
     callback.Run(query_result.GetWebResult());
     return;
   }
 
   if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
     DVLOG(1) << "Already connected.";
     callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
     return;
   }
 
   // It's possible for WebBluetoothServiceImpl to issue two successive
   // connection requests for which it would get two successive responses
   // and consequently try to insert two BluetoothGattConnections for the
   // same device. WebBluetoothServiceImpl should reject or queue connection
   // requests if there is a pending connection already, but the platform
   // abstraction doesn't currently support checking for pending connections.
   // TODO(ortuno): CHECK that this never happens once the platform
   // abstraction allows to check for pending connections.
   // http://crbug.com/583544
   const base::TimeTicks start_time = base::TimeTicks::Now();
+  blink::mojom::WebBluetoothServerClientAssociatedPtr server_client;

[dcheng, 2017/03/16 22:20:29]

server_client is kind of a confusing name here. *shrug*

[juncai, 2017/03/17 00:53:56]

Changed it to |web_bluetooth_server_client|.

Done.

+  server_client.Bind(std::move(client));
   query_result.device->CreateGattConnection(
       base::Bind(&WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess,
                  weak_ptr_factory_.GetWeakPtr(), device_id, start_time,
-                 callback),
+                 base::Passed(std::move(server_client)), callback),

[dcheng, 2017/03/16 22:20:29]

Nit: base::Passed(&server_client)

[juncai, 2017/03/17 00:53:56]

Done.

       base::Bind(&WebBluetoothServiceImpl::OnCreateGATTConnectionFailed,
                  weak_ptr_factory_.GetWeakPtr(), start_time, callback));
 }
 
 void WebBluetoothServiceImpl::RemoteServerDisconnect(
     const WebBluetoothDeviceId& device_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::REMOTE_GATT_SERVER_DISCONNECT);
 
@@ skipping 299 matching lines @@
   query_result.characteristic->WriteRemoteCharacteristic(
       value,
       base::Bind(&WebBluetoothServiceImpl::OnCharacteristicWriteValueSuccess,
                  weak_ptr_factory_.GetWeakPtr(), callback),
       base::Bind(&WebBluetoothServiceImpl::OnCharacteristicWriteValueFailed,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
     const std::string& characteristic_instance_id,
+    blink::mojom::WebBluetoothCharacteristicClientAssociatedPtrInfo client,
     const RemoteCharacteristicStartNotificationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_START_NOTIFICATIONS);
 
   auto iter =
       characteristic_id_to_notify_session_.find(characteristic_instance_id);
   if (iter != characteristic_id_to_notify_session_.end() &&
-      iter->second->IsActive()) {
+      iter->second->gatt_notify_session->IsActive()) {
     // If the frame has already started notifications and the notifications
     // are active we return SUCCESS.
     callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
     return;
   }
 
   const CacheQueryResult query_result =
       QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
     return;
   }
 
   if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
     RecordStartNotificationsOutcome(query_result.outcome);
     callback.Run(query_result.GetWebResult());
     return;
   }
 
   device::BluetoothRemoteGattCharacteristic::Properties notify_or_indicate =
       query_result.characteristic->GetProperties() &
       (device::BluetoothRemoteGattCharacteristic::PROPERTY_NOTIFY |
        device::BluetoothRemoteGattCharacteristic::PROPERTY_INDICATE);
   if (!notify_or_indicate) {
     callback.Run(blink::mojom::WebBluetoothResult::GATT_NOT_SUPPORTED);
     return;
   }
 
+  blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr
+      characteristic_client;
+  characteristic_client.Bind(std::move(client));
+
   query_result.characteristic->StartNotifySession(
       base::Bind(&WebBluetoothServiceImpl::OnStartNotifySessionSuccess,
-                 weak_ptr_factory_.GetWeakPtr(), callback),
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(std::move(characteristic_client)), callback),

[dcheng, 2017/03/16 22:20:29]

Ditto

[juncai, 2017/03/17 00:53:56]

Done.

       base::Bind(&WebBluetoothServiceImpl::OnStartNotifySessionFailed,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void WebBluetoothServiceImpl::RemoteCharacteristicStopNotifications(
     const std::string& characteristic_instance_id,
     const RemoteCharacteristicStopNotificationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::CHARACTERISTIC_STOP_NOTIFICATIONS);
 
   const CacheQueryResult query_result =
       QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
     return;
   }
 
   auto notify_session_iter =
       characteristic_id_to_notify_session_.find(characteristic_instance_id);
   if (notify_session_iter == characteristic_id_to_notify_session_.end()) {
     // If the frame hasn't subscribed to notifications before we just
     // run the callback.
     callback.Run();
     return;
   }
-  notify_session_iter->second->Stop(base::Bind(
+  notify_session_iter->second->gatt_notify_session->Stop(base::Bind(
       &WebBluetoothServiceImpl::OnStopNotifySessionComplete,
       weak_ptr_factory_.GetWeakPtr(), characteristic_instance_id, callback));
 }
 
 void WebBluetoothServiceImpl::RemoteDescriptorReadValue(
     const std::string& descriptor_instance_id,
     const RemoteDescriptorReadValueCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
       UMAWebBluetoothFunction::DESCRIPTOR_READ_VALUE);
@@ skipping 183 matching lines @@
     const RequestDeviceCallback& callback,
     blink::mojom::WebBluetoothResult result) {
   // Errors are recorded by the *device_chooser_controller_.
   callback.Run(result, nullptr /* device */);
   device_chooser_controller_.reset();
 }
 
 void WebBluetoothServiceImpl::OnCreateGATTConnectionSuccess(
     const WebBluetoothDeviceId& device_id,
     base::TimeTicks start_time,
+    blink::mojom::WebBluetoothServerClientAssociatedPtr client,
     const RemoteServerConnectCallback& callback,
     std::unique_ptr<device::BluetoothGattConnection> connection) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordConnectGATTTimeSuccess(base::TimeTicks::Now() - start_time);
   RecordConnectGATTOutcome(UMAConnectGATTOutcome::SUCCESS);
 
-  connected_devices_->Insert(device_id, std::move(connection));
+  if (connected_devices_->IsConnectedToDeviceWithId(device_id)) {
+    DVLOG(1) << "Already connected.";
+    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+    return;
+  }
+
   callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+  connected_devices_->Insert(device_id, std::move(connection),
+                             std::move(client));
 }
 
 void WebBluetoothServiceImpl::OnCreateGATTConnectionFailed(
     base::TimeTicks start_time,
     const RemoteServerConnectCallback& callback,
     device::BluetoothDevice::ConnectErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordConnectGATTTimeFailed(base::TimeTicks::Now() - start_time);
   callback.Run(TranslateConnectErrorAndRecord(error_code));
 }
@@ skipping 24 matching lines @@
 
 void WebBluetoothServiceImpl::OnCharacteristicWriteValueFailed(
     const RemoteCharacteristicWriteValueCallback& callback,
     device::BluetoothRemoteGattService::GattErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   callback.Run(TranslateGATTErrorAndRecord(
       error_code, UMAGATTOperation::CHARACTERISTIC_WRITE));
 }
 
 void WebBluetoothServiceImpl::OnStartNotifySessionSuccess(
+    blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr client,
     const RemoteCharacteristicStartNotificationsCallback& callback,
     std::unique_ptr<device::BluetoothGattNotifySession> notify_session) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Copy Characteristic Instance ID before passing a unique pointer because
   // compilers may evaluate arguments in any order.
   std::string characteristic_instance_id =
       notify_session->GetCharacteristicIdentifier();
+
+  auto iter =
+      characteristic_id_to_notify_session_.find(characteristic_instance_id);
+  if (iter != characteristic_id_to_notify_session_.end() &&
+      iter->second->gatt_notify_session->IsActive()) {

[dcheng, 2017/03/16 22:20:29]

Just to help me understand... what is IsActive(), and why do we check this?
Furthermore, why do we keep inactive sessions around in the map?

(I'm wondering how this interfaces with the Blink side - is it possible to end
up with multiple DOM objects that want to listen for changes on the same
characteristic?)

[juncai, 2017/03/17 00:53:56]

This is similar to:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

I'll try answering here.
scheib@, can you correct me if the following is incorrect? Thanks!

It is possible that when WebBluetoothServiceImpl::OnStartNotifySessionSuccess()
is called, BluetoothGattNotifySession::IsActive() returns false, since
BluetoothGattNotifySession::Stop() may just be called:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_gatt_notify_s...
but if BluetoothGattNotifySession::Stop()'s callback is not called yet:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
And the callback is supposed to remove the inactive session from the map:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

So in this case, the inactive session is still in the map. We still want to
update the new |notify_session| in the map. So we check IsActive() here.

But I think when the callback is called after the new |notify_session| is
updated in the map, this |notify_session| is removed. I saw there is already an
issue filed for this:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...

Based on this:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...
I think it is possible that multiple DOM objects listen for the same
characteristic.

[scheib, 2017/03/17 01:25:15]

Yes, the description for why IsActive may be false sounds accurate.

Thinking further though, I'm not sure why we need this logic block.

We hopefully already avoided requesting a duplicate session in
RemoteCharacteristicStartNotifications[1].

If we do somehow get a double success then we always want to return SUCCESS
anyway, and in both cases (active or not) it would be OK to replace the session
in the map with a new one.

So, probably we should remove this if -- unless juncai you can explain why we
need it?

[1]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

[juncai, 2017/03/17 17:52:59]

Done.

+    // If the frame has already started notifications and the notifications
+    // are active we return SUCCESS.
+    callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+    return;
+  }
+
+  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
   // Saving the BluetoothGattNotifySession keeps notifications active.
+  auto gatt_notify_session_and_client =
+      base::MakeUnique<GATTNotifySessionAndCharacteristicClient>(
+          std::move(notify_session), std::move(client));
   characteristic_id_to_notify_session_[characteristic_instance_id] =
-      std::move(notify_session);
-  callback.Run(blink::mojom::WebBluetoothResult::SUCCESS);
+      std::move(gatt_notify_session_and_client);
 }
 
 void WebBluetoothServiceImpl::OnStartNotifySessionFailed(
     const RemoteCharacteristicStartNotificationsCallback& callback,
     device::BluetoothRemoteGattService::GattErrorCode error_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   callback.Run(TranslateGATTErrorAndRecord(
       error_code, UMAGATTOperation::START_NOTIFICATIONS));
 }
 
@@ skipping 180 matching lines @@
   descriptor_id_to_characteristic_id_.clear();
   characteristic_id_to_service_id_.clear();
   service_id_to_device_address_.clear();
   connected_devices_.reset(
       new FrameConnectedBluetoothDevices(render_frame_host_));
   device_chooser_controller_.reset();
   BluetoothAdapterFactoryWrapper::Get().ReleaseAdapter(this);
 }
 
 }  // namespace content