Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

content/browser/bluetooth/web_bluetooth_service_impl.cc

Index: content/browser/bluetooth/web_bluetooth_service_impl.cc
diff --git a/content/browser/bluetooth/web_bluetooth_service_impl.cc b/content/browser/bluetooth/web_bluetooth_service_impl.cc
index f4bce42599a2dbff3057cf8d3c21a28ada910e01..73ca421ee710f524dd37e35cf46e21701635a836 100644
--- a/content/browser/bluetooth/web_bluetooth_service_impl.cc
+++ b/content/browser/bluetooth/web_bluetooth_service_impl.cc
@@ -12,6 +12,7 @@
 
 #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"
@@ -147,6 +148,18 @@ struct CacheQueryResult {
   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)
@@ -212,9 +225,6 @@ void WebBluetoothServiceImpl::DeviceChanged(device::BluetoothAdapter* adapter,
     base::Optional<WebBluetoothDeviceId> device_id =
         connected_devices_->CloseConnectionToDeviceWithAddress(
             device->GetAddress());
-    if (device_id && client_) {
-      client_->GattServerDisconnected(device_id.value());
-    }
   }
 }
 
@@ -271,18 +281,14 @@ void WebBluetoothServiceImpl::GattCharacteristicValueChanged(
 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) {
@@ -309,6 +315,7 @@ void WebBluetoothServiceImpl::RequestDevice(
 
 void WebBluetoothServiceImpl::RemoteServerConnect(
     const WebBluetoothDeviceId& device_id,
+    blink::mojom::WebBluetoothServerClientAssociatedPtrInfo client,
     const RemoteServerConnectCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(UMAWebBluetoothFunction::CONNECT_GATT);
@@ -337,10 +344,12 @@ void WebBluetoothServiceImpl::RemoteServerConnect(
   // 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));
 }
@@ -660,6 +669,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicWriteValue(
 
 void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
     const std::string& characteristic_instance_id,
+    blink::mojom::WebBluetoothCharacteristicClientAssociatedPtrInfo client,
     const RemoteCharacteristicStartNotificationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   RecordWebBluetoothFunctionCall(
@@ -668,7 +678,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
   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);
@@ -697,9 +707,14 @@ void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
     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));
 }
@@ -726,7 +741,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicStopNotifications(
     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));
 }
@@ -930,14 +945,22 @@ void WebBluetoothServiceImpl::OnGetDeviceFailed(
 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(
@@ -982,6 +1005,7 @@ void WebBluetoothServiceImpl::OnCharacteristicWriteValueFailed(
 }
 
 void WebBluetoothServiceImpl::OnStartNotifySessionSuccess(
+    blink::mojom::WebBluetoothCharacteristicClientAssociatedPtr client,
     const RemoteCharacteristicStartNotificationsCallback& callback,
     std::unique_ptr<device::BluetoothGattNotifySession> notify_session) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -989,10 +1013,24 @@ void WebBluetoothServiceImpl::OnStartNotifySessionSuccess(
   // 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(