Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.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/BluetoothRemoteGATTCharacteristic.h"
 
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
 #include "core/events/Event.h"
@@ skipping 13 matching lines @@
 namespace blink {
 
 BluetoothRemoteGATTCharacteristic::BluetoothRemoteGATTCharacteristic(
     ExecutionContext* context,
     mojom::blink::WebBluetoothRemoteGATTCharacteristicPtr characteristic,
     BluetoothRemoteGATTService* service,
     BluetoothDevice* device)
     : ContextLifecycleObserver(context),
       m_characteristic(std::move(characteristic)),
       m_service(service),
-      m_stopped(false),
+      m_clientBinding(this),
       m_device(device) {
   m_properties =
       BluetoothCharacteristicProperties::create(m_characteristic->properties);
 }
 
 BluetoothRemoteGATTCharacteristic* BluetoothRemoteGATTCharacteristic::create(
     ExecutionContext* context,
     mojom::blink::WebBluetoothRemoteGATTCharacteristicPtr characteristic,
     BluetoothRemoteGATTService* service,
     BluetoothDevice* device) {
   return new BluetoothRemoteGATTCharacteristic(
       context, std::move(characteristic), service, device);
 }
 
 void BluetoothRemoteGATTCharacteristic::setValue(DOMDataView* domDataView) {
   m_value = domDataView;
 }
 
-void BluetoothRemoteGATTCharacteristic::dispatchCharacteristicValueChanged(
-    const Vector<uint8_t>& value) {
+void BluetoothRemoteGATTCharacteristic::contextDestroyed(ExecutionContext*) {
+  notifyCharacteristicObjectRemoved();
+}
+
+void BluetoothRemoteGATTCharacteristic::RemoteCharacteristicValueChanged(
+    const WTF::Vector<uint8_t>& value) {
   if (!getGatt()->connected())
     return;
   this->setValue(BluetoothRemoteGATTUtils::ConvertWTFVectorToDataView(value));
   dispatchEvent(Event::create(EventTypeNames::characteristicvaluechanged));
 }
 
-void BluetoothRemoteGATTCharacteristic::contextDestroyed(ExecutionContext*) {
-  notifyCharacteristicObjectRemoved();
-}
-
 void BluetoothRemoteGATTCharacteristic::dispose() {
   notifyCharacteristicObjectRemoved();
 }
 
 void BluetoothRemoteGATTCharacteristic::notifyCharacteristicObjectRemoved() {

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

Can you change the name of this function? It doesn't seem appropriate now that
we are not notifying anyone that the object was removed.

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

Done.

-  if (!m_stopped) {
-    m_stopped = true;
-    m_device->bluetooth()->characteristicObjectRemoved(
-        m_characteristic->instance_id);
-  }
+  if (m_clientBinding.is_bound())
+    m_clientBinding.Close();
 }
 
 const WTF::AtomicString& BluetoothRemoteGATTCharacteristic::interfaceName()
     const {
   return EventTargetNames::BluetoothRemoteGATTCharacteristic;
 }
 
 ExecutionContext* BluetoothRemoteGATTCharacteristic::getExecutionContext()
     const {
   return ContextLifecycleObserver::getExecutionContext();
 }
 
 void BluetoothRemoteGATTCharacteristic::addedEventListener(
     const AtomicString& eventType,
     RegisteredEventListener& registeredListener) {
   EventTargetWithInlineData::addedEventListener(eventType, registeredListener);
-  // We will also need to unregister a characteristic once all the event
-  // listeners have been removed. See http://crbug.com/541390
-  if (eventType == EventTypeNames::characteristicvaluechanged) {

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

Forgot about this. We should keep an eye out for http://crbug.com/541390 as that
would mean we have to tie the client to this. Nothing to do for now.

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

Thanks!

-    m_device->bluetooth()->registerCharacteristicObject(
-        m_characteristic->instance_id, this);
-  }
 }
 
 void BluetoothRemoteGATTCharacteristic::ReadValueCallback(
     ScriptPromiseResolver* resolver,
     mojom::blink::WebBluetoothResult result,
     const Optional<Vector<uint8_t>>& value) {
   if (!resolver->getExecutionContext() ||
       resolver->getExecutionContext()->isContextDestroyed())
     return;
 
   // If the device is disconnected, reject.
   if (!getGatt()->RemoveFromActiveAlgorithms(resolver)) {
     resolver->reject(BluetoothError::createDOMException(
         blink::mojom::WebBluetoothResult::GATT_SERVER_DISCONNECTED));
     return;
   }
 
   if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
     DCHECK(value);
     DOMDataView* domDataView =
         BluetoothRemoteGATTUtils::ConvertWTFVectorToDataView(value.value());
     setValue(domDataView);
+    dispatchEvent(Event::create(EventTypeNames::characteristicvaluechanged));

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

This test
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
should make sure that the event is fired before the promise but it currently
just ignores the order. Could you please fix the test to make sure the event is
fire before the promise per step 5.5.4 of readValue()
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharac...

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

I opened an issue for it:
https://bugs.chromium.org/p/chromium/issues/detail?id=697698
And will fix it in a follow-up CL.

     resolver->resolve(domDataView);
   } else {
     resolver->reject(BluetoothError::createDOMException(result));
   }
 }
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::readValue(
     ScriptState* scriptState) {
   if (!getGatt()->connected()) {
     return ScriptPromise::rejectWithDOMException(
@@ skipping 85 matching lines @@
       m_characteristic->instance_id, valueVector,
       convertToBaseCallback(WTF::bind(
           &BluetoothRemoteGATTCharacteristic::WriteValueCallback,
           wrapPersistent(this), wrapPersistent(resolver), valueVector)));
 
   return promise;
 }
 
 void BluetoothRemoteGATTCharacteristic::NotificationsCallback(
     ScriptPromiseResolver* resolver,
-    mojom::blink::WebBluetoothResult result) {
+    mojom::blink::WebBluetoothResult result,
+    mojom::blink::WebBluetoothCharacteristicClientAssociatedRequest request) {
   if (!resolver->getExecutionContext() ||
       resolver->getExecutionContext()->isContextDestroyed())
     return;
 
   // If the device is disconnected, reject.
   if (!getGatt()->RemoveFromActiveAlgorithms(resolver)) {
     resolver->reject(BluetoothError::createDOMException(
         blink::mojom::WebBluetoothResult::GATT_SERVER_DISCONNECTED));
     return;
   }
 
+  m_clientBinding.Bind(std::move(request));
+
   if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
     resolver->resolve(this);
   } else {
     resolver->reject(BluetoothError::createDOMException(result));
   }
 }
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::startNotifications(
     ScriptState* scriptState) {
   if (!getGatt()->connected()) {
@@ skipping 41 matching lines @@
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
   getGatt()->AddToActiveAlgorithms(resolver);
 
   mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
   service->RemoteCharacteristicStopNotifications(
       m_characteristic->instance_id,
       convertToBaseCallback(
           WTF::bind(&BluetoothRemoteGATTCharacteristic::NotificationsCallback,
                     wrapPersistent(this), wrapPersistent(resolver),
-                    mojom::blink::WebBluetoothResult::SUCCESS)));
+                    mojom::blink::WebBluetoothResult::SUCCESS, nullptr)));
   return promise;
 }
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptor(
     ScriptState* scriptState,
     const StringOrUnsignedLong& descriptorUUID,
     ExceptionState& exceptionState) {
   String descriptor =
       BluetoothUUID::getDescriptor(descriptorUUID, exceptionState);
   if (exceptionState.hadException())
@@ skipping 121 matching lines @@
 DEFINE_TRACE(BluetoothRemoteGATTCharacteristic) {
   visitor->trace(m_service);
   visitor->trace(m_properties);
   visitor->trace(m_value);
   visitor->trace(m_device);
   EventTargetWithInlineData::trace(visitor);
   ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink