Remove RemoteServerDisconnect() from 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 19 matching lines @@
     ExecutionContext* context,
     BluetoothDevice* device) {
   return new BluetoothRemoteGATTServer(context, device);
 }
 
 void BluetoothRemoteGATTServer::contextDestroyed(ExecutionContext*) {
   Dispose();
 }
 
 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(
     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 (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) {
   visitor->trace(m_activeAlgorithms);
   visitor->trace(m_device);
   ContextLifecycleObserver::trace(visitor);
 }
 
 void BluetoothRemoteGATTServer::ConnectCallback(
     ScriptPromiseResolver* resolver,
@@ skipping 10 matching lines @@
   }
 }
 
 ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
 
   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(
           WTF::bind(&BluetoothRemoteGATTServer::ConnectCallback,
                     wrapPersistent(this), wrapPersistent(resolver))));
 
   return promise;
 }
 
 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
 // with a vector owning the services.
 void BluetoothRemoteGATTServer::GetPrimaryServicesCallback(
     const String& requestedServiceUUID,
     mojom::blink::WebBluetoothGATTQueryQuantity quantity,
     ScriptPromiseResolver* resolver,
     mojom::blink::WebBluetoothResult result,
     Optional<Vector<mojom::blink::WebBluetoothRemoteGATTServicePtr>> services) {
@@ skipping 90 matching lines @@
   service->RemoteServerGetPrimaryServices(
       m_device->id(), quantity, servicesUUID,
       convertToBaseCallback(
           WTF::bind(&BluetoothRemoteGATTServer::GetPrimaryServicesCallback,
                     wrapPersistent(this), servicesUUID, quantity,
                     wrapPersistent(resolver))));
   return promise;
 }
 
 }  // namespace blink