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..1d865c57f79c510aeddf408a833f1d7af23a6d4e 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,37 @@ bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
   return true;
 }
 
+void BluetoothRemoteGATTServer::disconnectIfConnected() {

[ortuno, 2017/03/01 04:52:06]

What do you think of the following:

void disconnect() {
  ClearActiveAlgorithms();
  if (!connected()) {
    return;
  }

  cleanupDisconnectedDevice();
  m_device->dispatchEvent(...);

  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
  service->RemoteServerDisconnect(device()->id());

  DCHECK(m_clientBinding.is_bound());
  m_clientBinding.Close();
}

void GATTServerDisconnected() {
  // This function can only be called if m_clientBinding is bound and
m_clientBinding can
  // only be bound if connected() is true. Therefore we skip Step 2 of
Responding to
  // Disconnection.
https://webbluetoothcg.github.io/web-bluetooth/#disconnection-events
  cleanupDisconnectedDevice();
  m_device->dispatchEvent(...);
}

void cleanupDisconnectedDevice() {
  DCHECK(connected());
  connected_ = false;
  m_activeAlgorithms.clear();
  m_device->clearAttributeInstanceMap();
}


void dispose() {
  disconnectIfConnected();
}

void contextDestroyed(...) {
  disconnectIfConnected();
}

void disconnectIfConnected() {
  if (!connected())
    return;

  cleanupDisconnectedDevice();
  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
  service->RemoteServerDisconnect(device()->id());
  DCHECK(m_clientBinding.is_bound())
  m_clientBinding.Close();
}

[juncai, 2017/03/02 03:23:49]

I think this change can be combined in a follow-up CL that also removes the
RemoteServerDisconnect() from web_bluetooth.mojom that you suggested in a
previous comment.
I opened an issue for that:
https://bugs.chromium.org/p/chromium/issues/detail?id=697700

+  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() {
+  // 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 +92,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/01 04:52:06]

Why not just send a nullptr for already connected devices?

[juncai, 2017/03/02 03:23:49]

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 +125,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());
 }