Remove RemoteServerDisconnect() from 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 ed0fe4bdfaa440ee1ad0254bdab392de017315d2..48456cb4ab3d8e3853b8ee19d46e71059dd93518 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
@@ -37,7 +37,12 @@ void BluetoothRemoteGATTServer::contextDestroyed(ExecutionContext*) {
 }
 
 void BluetoothRemoteGATTServer::GATTServerDisconnected() {
-  DispatchDisconnected();
+  // This function can only be called if |m_clientBinding| is bound and
+  // |m_clientBinding| can only be bound if |m_connected| is true.
+  // Therefore we skip Step 2 of Responding to Disconnection.
+  // https://webbluetoothcg.github.io/web-bluetooth/#disconnection-events
+  DCHECK(m_clientBinding.is_bound());
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
 }
 
 void BluetoothRemoteGATTServer::AddToActiveAlgorithms(
@@ -55,34 +60,32 @@ bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
   return true;
 }
 
-void BluetoothRemoteGATTServer::DisconnectIfConnected() {
-  if (m_connected) {
-    SetConnected(false);
-    ClearActiveAlgorithms();
-    mojom::blink::WebBluetoothService* service =
-        m_device->bluetooth()->Service();
-    service->RemoteServerDisconnect(m_device->id());
-  }
-}
-
-void BluetoothRemoteGATTServer::CleanupDisconnectedDeviceAndFireEvent() {
+void BluetoothRemoteGATTServer::CleanupDisconnectedDevice(bool dispatchEvent) {

[scheib, 2017/03/22 01:11:08]

ortuno reminds us that this implementation is intended to follow
https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device.


Please add a comment on this implementation to make that clear

// This implements the specification "clean up the disconnected device"
algorithm 
//
https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device

I think it is warranted in both declaration and here in definition because it's
being used in a very specific way.

[juncai, 2017/03/27 20:44:46]

Done.

   DCHECK(m_connected);
   SetConnected(false);
   ClearActiveAlgorithms();
-  m_device->ClearAttributeInstanceMapAndFireEvent();
+  m_device->ClearAttributeInstanceMap();

[scheib, 2017/03/22 01:11:08]

Below this line there are steps in the specification which we are not explicitly
handling here (clearing representedService, notification & indications). We
should comment here explaining why no explicit actions are needed. OK to mark
this as a TODO and follow up.

[juncai, 2017/03/27 20:44:46]

Done.

+  if (dispatchEvent) {
+    m_device->dispatchEvent(
+        Event::createBubble(EventTypeNames::gattserverdisconnected));
+  }
 }
 
-void BluetoothRemoteGATTServer::DispatchDisconnected() {
+void BluetoothRemoteGATTServer::HandleClientConnectionError() {
   if (!m_connected) {
     return;
   }
-  CleanupDisconnectedDeviceAndFireEvent();
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
 }
 
 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_connected) {
+    return;
+  }
+  // The object is being garbage collected or the context is being destroyed,
+  // so no need to dispatch a gattserverdisconnected event.
+  CleanupDisconnectedDevice(false /* dispatchEvent */);

[scheib, 2017/03/22 01:11:08]

Thanks! So much clearer.

+  DCHECK(m_clientBinding.is_bound());
   m_clientBinding.Close();
 }
 
@@ -114,6 +117,9 @@ ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
   mojom::blink::WebBluetoothService* service = m_device->bluetooth()->Service();
   mojom::blink::WebBluetoothServerClientAssociatedPtrInfo ptrInfo;
   m_clientBinding.Bind(&ptrInfo);
+  m_clientBinding.set_connection_error_handler(convertToBaseCallback(
+      WTF::bind(&BluetoothRemoteGATTServer::HandleClientConnectionError,
+                wrapWeakPersistent(this))));
   service->RemoteServerConnect(
       m_device->id(), std::move(ptrInfo),
       convertToBaseCallback(
@@ -124,12 +130,13 @@ ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
 }
 
 void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
-  if (!m_connected)
+  ClearActiveAlgorithms();
+  if (!m_connected) {
     return;
-  CleanupDisconnectedDeviceAndFireEvent();
+  }
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
+  DCHECK(m_clientBinding.is_bound());
   m_clientBinding.Close();

[scheib, 2017/03/22 01:11:08]

It now becomes less clear how we are signaling to the WBService that disconnect
should happen. Perhaps a short comment here, or less so on the member
m_clientBinding.

// Implicitly signals disconnect via the connection_error_handler

[juncai, 2017/03/27 20:44:46]

Done.

-  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->Service();
-  service->RemoteServerDisconnect(m_device->id());
 }
 
 // Callback that allows us to resolve the promise with a single service or