Refactor WebBluetoothServiceClient in the web_bluetooth.mojom

third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp

Index: third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
diff --git a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
index 0088df16c6780168063af4f5f0230a209b6ed3f5..a9fe0119f43a6056d7e4af859dc3bc2c1e027502 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
@@ -31,7 +31,7 @@ BluetoothRemoteGATTCharacteristic::BluetoothRemoteGATTCharacteristic(
     : 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);
@@ -50,28 +50,25 @@ 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()
@@ -88,12 +85,6 @@ 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(
@@ -116,6 +107,7 @@ void BluetoothRemoteGATTCharacteristic::ReadValueCallback(
     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));
@@ -221,7 +213,8 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::writeValue(
 
 void BluetoothRemoteGATTCharacteristic::NotificationsCallback(
     ScriptPromiseResolver* resolver,
-    mojom::blink::WebBluetoothResult result) {
+    mojom::blink::WebBluetoothResult result,
+    mojom::blink::WebBluetoothCharacteristicClientAssociatedRequest request) {
   if (!resolver->getExecutionContext() ||
       resolver->getExecutionContext()->isContextDestroyed())
     return;
@@ -233,6 +226,8 @@ void BluetoothRemoteGATTCharacteristic::NotificationsCallback(
     return;
   }
 
+  m_clientBinding.Bind(std::move(request));
+
   if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
     resolver->resolve(this);
   } else {
@@ -294,7 +289,7 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::stopNotifications(
       convertToBaseCallback(
           WTF::bind(&BluetoothRemoteGATTCharacteristic::NotificationsCallback,
                     wrapPersistent(this), wrapPersistent(resolver),
-                    mojom::blink::WebBluetoothResult::SUCCESS)));
+                    mojom::blink::WebBluetoothResult::SUCCESS, nullptr)));
   return promise;
 }