Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

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

 // Copyright 2015 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 "modules/bluetooth/BluetoothRemoteGATTServer.h"
 
 #include "bindings/core/v8/CallbackPromiseAdapter.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
@@ skipping 10 matching lines @@
 
 namespace {
 
 const char kGATTServerNotConnected[] =
     "GATT Server is disconnected. Cannot retrieve services. (Re)connect first "
     "with `device.gatt.connect`.";
 
 }  // 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);
   CHECK(result.isNewEntry);
 }
 
 bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
     ScriptPromiseResolver* resolver) {
   if (!m_activeAlgorithms.contains(resolver)) {
     return false;
   }
   m_activeAlgorithms.erase(resolver);
   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);
 }
 
 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 {
     resolver->reject(BluetoothError::createDOMException(result));
   }
 }
 
 ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
 
   mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
   service->RemoteServerConnect(
       device()->id(), convertToBaseCallback(WTF::bind(
                           &BluetoothRemoteGATTServer::ConnectCallback,
                           wrapPersistent(this), wrapPersistent(resolver))));
 
   return promise;
 }
 
 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());
 }
 
 // Callback that allows us to resolve the promise with a single service or
 // with a vector owning the services.
 void BluetoothRemoteGATTServer::GetPrimaryServicesCallback(
     const String& requestedServiceUUID,
     mojom::blink::WebBluetoothGATTQueryQuantity quantity,
     ScriptPromiseResolver* resolver,
@@ skipping 92 matching lines @@
   service->RemoteServerGetPrimaryServices(
       device()->id(), quantity, servicesUUID,
       convertToBaseCallback(
           WTF::bind(&BluetoothRemoteGATTServer::GetPrimaryServicesCallback,
                     wrapPersistent(this), servicesUUID, quantity,
                     wrapPersistent(resolver))));
   return promise;
 }
 
 }  // namespace blink