Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp

Index: third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
diff --git a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
index 8b2185a27894517466375353468015d90f583b4b..8c9e0d0661a64b0dc64a749be8cad126e758cb3b 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
@@ -28,13 +28,17 @@ const char kGATTServerNotConnected[] =
 }  // namespace
 
 BluetoothRemoteGATTServer::BluetoothRemoteGATTServer(BluetoothDevice* device)
-    : m_device(device), m_connected(false) {}
+    : m_clientBinding(this), m_device(device), m_connected(false) {}
 
 BluetoothRemoteGATTServer* BluetoothRemoteGATTServer::create(
     BluetoothDevice* device) {
   return new BluetoothRemoteGATTServer(device);
 }
 
+void BluetoothRemoteGATTServer::GATTServerDisconnected() {
+  dispatchDisconnected();
+}
+
 void BluetoothRemoteGATTServer::AddToActiveAlgorithms(
     ScriptPromiseResolver* resolver) {
   auto result = m_activeAlgorithms.insert(resolver);
@@ -50,6 +54,38 @@ bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
   return true;
 }
 
+void BluetoothRemoteGATTServer::disconnectIfConnected() {
+  if (connected()) {
+    setConnected(false);
+    ClearActiveAlgorithms();
+    mojom::blink::WebBluetoothService* service =
+        m_device->bluetooth()->service();
+    service->RemoteServerDisconnect(m_device->id());
+  }
+}
+
+void BluetoothRemoteGATTServer::cleanupDisconnectedDeviceAndFireEvent() {
+  DCHECK(connected());
+  setConnected(false);
+  ClearActiveAlgorithms();
+  m_device->clearAttributeInstanceMapAndFireEvent();
+}
+
+void BluetoothRemoteGATTServer::dispatchDisconnected() {
+  if (!connected()) {
+    return;
+  }
+  cleanupDisconnectedDeviceAndFireEvent();
+}
+
+void BluetoothRemoteGATTServer::dispose() {
+  disconnectIfConnected();
+  // The pipe to this object must be closed when is marked unreachable to
+  // prevent messages from being dispatched before lazy sweeping.
+  if (m_clientBinding.is_bound())
+    m_clientBinding.Close();
+}
+
 DEFINE_TRACE(BluetoothRemoteGATTServer) {
   visitor->trace(m_activeAlgorithms);
   visitor->trace(m_device);
@@ -57,13 +93,16 @@ DEFINE_TRACE(BluetoothRemoteGATTServer) {
 
 void BluetoothRemoteGATTServer::ConnectCallback(
     ScriptPromiseResolver* resolver,
-    mojom::blink::WebBluetoothResult result) {
+    mojom::blink::WebBluetoothResult result,
+    mojom::blink::WebBluetoothServerClientAssociatedRequest request) {
   if (!resolver->getExecutionContext() ||
       resolver->getExecutionContext()->isContextDestroyed())
     return;
 
+  if (!m_connected)

[ortuno, 2017/03/06 11:31:20]

Binding the request based on tha value of m_connected forces readers to think of
all the cases in which m_connected could be true and makes the intent harder to
understand i.e. we only bind valid requests. Furthermore m_connected and binding
seem like two unrelated operations which could lead to bugs. Case in point:

gatt.connect().then(() => gatt.disconnect());
gatt.connect();

1. Dispatch two connection requests.
2. First connect response arrives to the browser so
OnCreateGATTConnectionSuccess is called. Since there are no other connections in
the map, the ptr is added to the map, we create request based on the ptr and
sent it to the renderer.
3. Second connect response arrives to the browser so
OnCreateGATTConnectionSuccess is called. Since there is already a connection in
the map we just send nullptr.
4. First connect response arrives on the renderer. We bind the request and set
m_connected to true and resolve the promise.
5. Now that the promise has resolve we call gatt.disconnect() which sets
m_connected to false.
6. Second connect response arrives on the render. Since m_connected is false we
bind the invalid request :(

Change this to:

// WebBluetoothService sends an invalid request and SUCCESS if there is a client
// already.
if (request.is_valid())
  m_clientBinding.Bind(std::move(request));

And also please move inside SUCCESS if statement to make it more obvious that
the request should be bound on success.

[juncai, 2017/03/09 07:30:57]

Done.

+    m_clientBinding.Bind(std::move(request));
+
   if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
-    m_device->bluetooth()->addToConnectedDevicesMap(device()->id(), device());
     setConnected(true);
     resolver->resolve(this);
   } else {
@@ -87,8 +126,7 @@ ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
 void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
   if (!m_connected)
     return;
-  device()->cleanupDisconnectedDeviceAndFireEvent();
-  m_device->bluetooth()->removeFromConnectedDevicesMap(device()->id());
+  cleanupDisconnectedDeviceAndFireEvent();
   mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
   service->RemoteServerDisconnect(device()->id());
 }