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..03c9866329fbfcf1b787457e379097d5b889c553 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp
@@ -37,7 +37,13 @@ void BluetoothRemoteGATTServer::contextDestroyed(ExecutionContext*) {
 }
 
 void BluetoothRemoteGATTServer::GATTServerDisconnected() {
-  DispatchDisconnected();
+  // This function can only be called if m_clientBinding is bound and

[scheib, 2017/03/20 21:39:14]

Can we assert these claims with DCHECKs?

[juncai, 2017/03/21 00:52:35]

Added a "DCHECK(m_clientBinding.is_bound());" here.
And CleanupDisconnectedDevice() has "DCHECK(m_connected);".

Done.

+  // 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
+  CleanupDisconnectedDevice();
+  m_device->dispatchEvent(

[scheib, 2017/03/20 21:39:14]

Nearly all locations CleanupDisconnectedDevice is called create this event.
Should it be created in CleanupDisconnectedDevice? Perhaps an argument to
control that? Let's understand why we handle it differently when we don't, and
make that clear.

[juncai, 2017/03/21 00:52:35]

Added an extra parameter to CleanupDisconnectedDevice() and it controls if
dispatching an event.

Also added comments to places where no event is dispatched.

Done.

+      Event::createBubble(EventTypeNames::gattserverdisconnected));
 }
 
 void BluetoothRemoteGATTServer::AddToActiveAlgorithms(
@@ -56,34 +62,34 @@ bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
 }
 
 void BluetoothRemoteGATTServer::DisconnectIfConnected() {
-  if (m_connected) {
-    SetConnected(false);
-    ClearActiveAlgorithms();
-    mojom::blink::WebBluetoothService* service =
-        m_device->bluetooth()->Service();
-    service->RemoteServerDisconnect(m_device->id());
+  if (!m_connected) {
+    return;
   }
+
+  CleanupDisconnectedDevice();

[scheib, 2017/03/20 21:39:14]

This location doesn't create a disconnected event. Let's understand why not and
explain it.

[juncai, 2017/03/21 00:52:35]

This function DisconnectIfConnected() is removed since it is only called by
Dispose(), so moved its content to Dispose().

And added comment there to explain why no event is dispatched.

Done.

+  DCHECK(m_clientBinding.is_bound());
+  m_clientBinding.Close();
 }
 
-void BluetoothRemoteGATTServer::CleanupDisconnectedDeviceAndFireEvent() {
+void BluetoothRemoteGATTServer::CleanupDisconnectedDevice() {
   DCHECK(m_connected);
   SetConnected(false);
   ClearActiveAlgorithms();
-  m_device->ClearAttributeInstanceMapAndFireEvent();
+  m_device->ClearAttributeInstanceMap();
 }
 
-void BluetoothRemoteGATTServer::DispatchDisconnected() {
+void BluetoothRemoteGATTServer::HandleClientConnectionError() {
   if (!m_connected) {
     return;
   }
-  CleanupDisconnectedDeviceAndFireEvent();
+
+  CleanupDisconnectedDevice();
+  m_device->dispatchEvent(
+      Event::createBubble(EventTypeNames::gattserverdisconnected));
 }
 
 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.
-  m_clientBinding.Close();
 }
 
 DEFINE_TRACE(BluetoothRemoteGATTServer) {
@@ -114,6 +120,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 +133,17 @@ ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
 }
 
 void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
-  if (!m_connected)
+  ClearActiveAlgorithms();
+  if (!m_connected) {
     return;
-  CleanupDisconnectedDeviceAndFireEvent();
+  }
+
+  CleanupDisconnectedDevice();
+  m_device->dispatchEvent(
+      Event::createBubble(EventTypeNames::gattserverdisconnected));
+
+  DCHECK(m_clientBinding.is_bound());
   m_clientBinding.Close();
-  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