[Onion Soup] Move WebBluetoothImpl from //content/renderer/bluetooth to Blink's bluetooth module

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 7f228d893433c15ec228ac4f358577839ea05ba0..51087b839142a05f4eea2edfa5ac14c7cbf9b99c 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
@@ -11,11 +11,11 @@
 #include "core/dom/ExceptionCode.h"
 #include "core/events/Event.h"
 #include "core/inspector/ConsoleMessage.h"
+#include "modules/bluetooth/Bluetooth.h"
 #include "modules/bluetooth/BluetoothCharacteristicProperties.h"
+#include "modules/bluetooth/BluetoothDevice.h"
 #include "modules/bluetooth/BluetoothError.h"
 #include "modules/bluetooth/BluetoothRemoteGATTService.h"
-#include "modules/bluetooth/BluetoothSupplement.h"
-#include "public/platform/modules/bluetooth/WebBluetooth.h"
 #include <memory>
 
 namespace blink {
@@ -30,37 +30,48 @@ const char kInvalidCharacteristic[] =
     "Characteristic is no longer valid. Remember to retrieve the "
     "characteristic again after reconnecting.";
 
-DOMDataView* ConvertWebVectorToDataView(const WebVector<uint8_t>& webVector) {
-  static_assert(sizeof(*webVector.data()) == 1,
+DOMDataView* ConvertWTFVectorToDataView(const Vector<uint8_t>& wtfVector) {
+  static_assert(sizeof(*wtfVector.data()) == 1,
                 "uint8_t should be a single byte");
   DOMArrayBuffer* domBuffer =
-      DOMArrayBuffer::create(webVector.data(), webVector.size());
-  return DOMDataView::create(domBuffer, 0, webVector.size());
+      DOMArrayBuffer::create(wtfVector.data(), wtfVector.size());
+  return DOMDataView::create(domBuffer, 0, wtfVector.size());
 }
 
 }  // anonymous namespace
 
 BluetoothRemoteGATTCharacteristic::BluetoothRemoteGATTCharacteristic(
     ExecutionContext* context,
-    std::unique_ptr<WebBluetoothRemoteGATTCharacteristicInit> webCharacteristic,
-    BluetoothRemoteGATTService* service)
+    const String& characteristicInstanceId,
+    const String& serviceInstanceId,
+    const String& uuid,
+    uint32_t characteristicProperties,
+    BluetoothRemoteGATTService* service,
+    BluetoothDevice* device)
     : SuspendableObject(context),
-      m_webCharacteristic(std::move(webCharacteristic)),
+      m_characteristicInstanceId(characteristicInstanceId),
+      m_serviceInstanceId(serviceInstanceId),
+      m_uuid(uuid),
+      m_characteristicProperties(characteristicProperties),
       m_service(service),
-      m_stopped(false) {
-  m_properties = BluetoothCharacteristicProperties::create(
-      m_webCharacteristic->characteristicProperties);
+      m_stopped(false),
+      m_device(device) {
+  m_properties =
+      BluetoothCharacteristicProperties::create(m_characteristicProperties);
 }
 
 BluetoothRemoteGATTCharacteristic* BluetoothRemoteGATTCharacteristic::create(
     ExecutionContext* context,
-    std::unique_ptr<WebBluetoothRemoteGATTCharacteristicInit> webCharacteristic,
-    BluetoothRemoteGATTService* service) {
-  DCHECK(webCharacteristic);
-
+    const String& characteristicInstanceId,
+    const String& serviceInstanceId,
+    const String& uuid,
+    uint32_t characteristicProperties,
+    BluetoothRemoteGATTService* service,
+    BluetoothDevice* device) {
   BluetoothRemoteGATTCharacteristic* characteristic =
       new BluetoothRemoteGATTCharacteristic(
-          context, std::move(webCharacteristic), service);
+          context, characteristicInstanceId, serviceInstanceId, uuid,
+          characteristicProperties, service, device);
   // See note in SuspendableObject about suspendIfNeeded.
   characteristic->suspendIfNeeded();
   return characteristic;
@@ -71,8 +82,8 @@ void BluetoothRemoteGATTCharacteristic::setValue(DOMDataView* domDataView) {
 }
 
 void BluetoothRemoteGATTCharacteristic::dispatchCharacteristicValueChanged(
-    const WebVector<uint8_t>& value) {
-  this->setValue(ConvertWebVectorToDataView(value));
+    const Vector<uint8_t>& value) {
+  this->setValue(ConvertWTFVectorToDataView(value));
   dispatchEvent(Event::create(EventTypeNames::characteristicvaluechanged));
 }
 
@@ -87,10 +98,8 @@ void BluetoothRemoteGATTCharacteristic::dispose() {
 void BluetoothRemoteGATTCharacteristic::notifyCharacteristicObjectRemoved() {
   if (!m_stopped) {
     m_stopped = true;
-    WebBluetooth* webbluetooth = BluetoothSupplement::fromExecutionContext(
-        SuspendableObject::getExecutionContext());
-    webbluetooth->characteristicObjectRemoved(
-        m_webCharacteristic->characteristicInstanceID, this);
+    m_device->bluetooth()->characteristicObjectRemoved(
+        m_characteristicInstanceId);
   }
 }
 
@@ -111,65 +120,40 @@ void BluetoothRemoteGATTCharacteristic::addedEventListener(
   // 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) {
-    WebBluetooth* webbluetooth =
-        BluetoothSupplement::fromExecutionContext(getExecutionContext());
-    webbluetooth->registerCharacteristicObject(
-        m_webCharacteristic->characteristicInstanceID, this);
+    m_device->bluetooth()->registerCharacteristicObject(
+        m_characteristicInstanceId, this);
   }
 }
 
-class ReadValueCallback : public WebBluetoothReadValueCallbacks {
- public:
-  ReadValueCallback(BluetoothRemoteGATTCharacteristic* characteristic,
-                    ScriptPromiseResolver* resolver)
-      : m_characteristic(characteristic), m_resolver(resolver) {
-    // We always check that the device is connected before constructing this
-    // object.
-    CHECK(m_characteristic->gatt()->connected());
-    m_characteristic->gatt()->AddToActiveAlgorithms(m_resolver.get());
-  }
-
-  void onSuccess(const WebVector<uint8_t>& value) override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+void BluetoothRemoteGATTCharacteristic::ReadValueCallback(
+    ScriptPromiseResolver* resolver,
+    mojom::blink::WebBluetoothResult result,
+    const Optional<Vector<uint8_t>>& value) {
+  if (!resolver->getExecutionContext() ||
+      resolver->getExecutionContext()->isContextDestroyed())
+    return;

[Reilly Grant (use Gerrit), 2016/12/16 01:41:29]

This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

[juncai, 2016/12/17 01:18:52]

Keep this check if the callbacks do more than
ScriptPromiseResolver::resolve/reject.

+
+  if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
+    DCHECK(value);
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    DOMDataView* domDataView = ConvertWebVectorToDataView(value);
-    if (m_characteristic)
-      m_characteristic->setValue(domDataView);
-
-    m_resolver->resolve(domDataView);
-  }
-
-  void onError(
-      int32_t
-          error /* Corresponds to WebBluetoothResult in web_bluetooth.mojom */)
-      override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+    DOMDataView* domDataView = ConvertWTFVectorToDataView(value.value());
+    setValue(domDataView);
+    resolver->resolve(domDataView);
+  } else {
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    m_resolver->reject(BluetoothError::take(m_resolver, error));
+    resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));

[dougt, 2016/12/16 00:11:44]

It would be great if BluetoothError::take would accept a WebBluetoothResult so
that we can avoid the cast?

[juncai, 2016/12/17 01:18:52]

Done.

   }
-
- private:
-  Persistent<BluetoothRemoteGATTCharacteristic> m_characteristic;
-  Persistent<ScriptPromiseResolver> m_resolver;
-};
+}
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::readValue(
     ScriptState* scriptState) {
@@ -179,75 +163,56 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::readValue(
         DOMException::create(NetworkError, kGATTServerNotConnected));
   }
 
-  if (!gatt()->device()->isValidCharacteristic(
-          m_webCharacteristic->characteristicInstanceID)) {
+  if (!gatt()->device()->isValidCharacteristic(m_characteristicInstanceId)) {
     return ScriptPromise::rejectWithDOMException(
         scriptState,
         DOMException::create(InvalidStateError, kInvalidCharacteristic));
   }
 
-  WebBluetooth* webbluetooth =
-      BluetoothSupplement::fromScriptState(scriptState);
-
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
-  webbluetooth->readValue(m_webCharacteristic->characteristicInstanceID,
-                          new ReadValueCallback(this, resolver));
 
-  return promise;
-}
+  // We always check that the device is connected.
+  CHECK(gatt()->connected());
+  gatt()->AddToActiveAlgorithms(resolver);
 
-class WriteValueCallback : public WebBluetoothWriteValueCallbacks {
- public:
-  WriteValueCallback(BluetoothRemoteGATTCharacteristic* characteristic,
-                     ScriptPromiseResolver* resolver)
-      : m_characteristic(characteristic), m_resolver(resolver) {
-    // We always check that the device is connected before constructing this
-    // object.
-    CHECK(m_characteristic->gatt()->connected());
-    m_characteristic->gatt()->AddToActiveAlgorithms(m_resolver.get());
-  }
+  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
+  service->RemoteCharacteristicReadValue(
+      m_characteristicInstanceId,
+      convertToBaseCallback(
+          WTF::bind(&BluetoothRemoteGATTCharacteristic::ReadValueCallback,
+                    wrapPersistent(this), wrapPersistent(resolver))));
 
-  void onSuccess(const WebVector<uint8_t>& value) override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
+  return promise;
+}
 
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+void BluetoothRemoteGATTCharacteristic::WriteValueCallback(
+    ScriptPromiseResolver* resolver,
+    const Vector<uint8_t>& value,
+    mojom::blink::WebBluetoothResult result) {
+  if (!resolver->getExecutionContext() ||
+      resolver->getExecutionContext()->isContextDestroyed())
+    return;

[Reilly Grant (use Gerrit), 2016/12/16 01:41:29]

This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

[juncai, 2016/12/17 01:18:52]

ditto.

+
+  if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    if (m_characteristic) {
-      m_characteristic->setValue(ConvertWebVectorToDataView(value));
-    }
-    m_resolver->resolve();
-  }
-
-  void onError(
-      int32_t
-          error /* Corresponds to WebBluetoothResult in web_bluetooth.mojom */)
-      override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+    setValue(ConvertWTFVectorToDataView(value));
+    resolver->resolve();
+  } else {
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    m_resolver->reject(BluetoothError::take(m_resolver, error));
+    resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));

[dougt, 2016/12/16 00:11:44]

same

[juncai, 2016/12/17 01:18:52]

Done.

   }
-
- private:
-  Persistent<BluetoothRemoteGATTCharacteristic> m_characteristic;
-  Persistent<ScriptPromiseResolver> m_resolver;
-};
+}
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::writeValue(
     ScriptState* scriptState,
@@ -258,15 +223,12 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::writeValue(
         DOMException::create(NetworkError, kGATTServerNotConnected));
   }
 
-  if (!gatt()->device()->isValidCharacteristic(
-          m_webCharacteristic->characteristicInstanceID)) {
+  if (!gatt()->device()->isValidCharacteristic(m_characteristicInstanceId)) {
     return ScriptPromise::rejectWithDOMException(
         scriptState,
         DOMException::create(InvalidStateError, kInvalidCharacteristic));
   }
 
-  WebBluetooth* webbluetooth =
-      BluetoothSupplement::fromScriptState(scriptState);
   // Partial implementation of writeValue algorithm:
   // https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattcharacteristic-writevalue
 
@@ -279,65 +241,51 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::writeValue(
                                           "Value can't exceed 512 bytes."));
 
   // Let valueVector be a copy of the bytes held by value.
-  WebVector<uint8_t> valueVector(value.bytes(), value.byteLength());
+  Vector<uint8_t> valueVector;
+  valueVector.append(value.bytes(), value.byteLength());

[dougt, 2016/12/16 00:11:44]

Do we need this change?

[juncai, 2016/12/17 01:18:52]

I think so.
https://cs.chromium.org/chromium/src/out/Debug/gen/third_party/WebKit/public/...
The WebBluetoothService::RemoteCharacteristicWriteValue() takes a WTF::Vector as
a parameter instead of a WebVector.

 
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
-
   ScriptPromise promise = resolver->promise();
-  webbluetooth->writeValue(m_webCharacteristic->characteristicInstanceID,
-                           valueVector, new WriteValueCallback(this, resolver));
+
+  // We always check that the device is connected.
+  CHECK(gatt()->connected());

[dougt, 2016/12/16 00:11:44]

this is a crash if we're not connected here. Should we instead handle this case?

[juncai, 2016/12/17 01:18:52]

I think this line can just be removed since this case is already handled on line
220.

Done.

+  gatt()->AddToActiveAlgorithms(resolver);
+
+  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
+  service->RemoteCharacteristicWriteValue(
+      m_characteristicInstanceId, valueVector,
+      convertToBaseCallback(WTF::bind(
+          &BluetoothRemoteGATTCharacteristic::WriteValueCallback,
+          wrapPersistent(this), wrapPersistent(resolver), valueVector)));
 
   return promise;
 }
 
-class NotificationsCallback : public WebBluetoothNotificationsCallbacks {
- public:
-  NotificationsCallback(BluetoothRemoteGATTCharacteristic* characteristic,
-                        ScriptPromiseResolver* resolver)
-      : m_characteristic(characteristic), m_resolver(resolver) {
-    // We always check that the device is connected before constructing this
-    // object.
-    CHECK(m_characteristic->gatt()->connected());
-    m_characteristic->gatt()->AddToActiveAlgorithms(m_resolver.get());
-  }
-
-  void onSuccess() override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
+void BluetoothRemoteGATTCharacteristic::NotificationsCallback(
+    ScriptPromiseResolver* resolver,
+    mojom::blink::WebBluetoothResult result) {
+  if (!resolver->getExecutionContext() ||
+      resolver->getExecutionContext()->isContextDestroyed())
+    return;

[Reilly Grant (use Gerrit), 2016/12/16 01:41:29]

This check is unnecessary. It is performed by
ScriptPromiseResolver::resolve/reject.

[juncai, 2016/12/17 01:18:52]

ditto.

 
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+  if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    m_resolver->resolve(m_characteristic);
-  }
-
-  void onError(
-      int32_t
-          error /* Corresponds to WebBluetoothResult in web_bluetooth.mojom */)
-      override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
+    resolver->resolve(this);
+  } else {
+    if (!gatt()->RemoveFromActiveAlgorithms(resolver)) {
+      resolver->reject(
           DOMException::create(NetworkError, kGATTServerDisconnected));
       return;
     }
 
-    m_resolver->reject(BluetoothError::take(m_resolver, error));
+    resolver->reject(BluetoothError::take(resolver, static_cast<int>(result)));

[dougt, 2016/12/16 00:11:44]

same as above.  Would be nice if ::take() accepted WebBluetoothResult.

[juncai, 2016/12/17 01:18:52]

Done.

   }
-
- private:
-  Persistent<BluetoothRemoteGATTCharacteristic> m_characteristic;
-  Persistent<ScriptPromiseResolver> m_resolver;
-};
+}
 
 ScriptPromise BluetoothRemoteGATTCharacteristic::startNotifications(
     ScriptState* scriptState) {
@@ -347,20 +295,26 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::startNotifications(
         DOMException::create(NetworkError, kGATTServerNotConnected));
   }
 
-  if (!gatt()->device()->isValidCharacteristic(
-          m_webCharacteristic->characteristicInstanceID)) {
+  if (!gatt()->device()->isValidCharacteristic(m_characteristicInstanceId)) {
     return ScriptPromise::rejectWithDOMException(
         scriptState,
         DOMException::create(InvalidStateError, kInvalidCharacteristic));
   }
 
-  WebBluetooth* webbluetooth =
-      BluetoothSupplement::fromScriptState(scriptState);
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
-  webbluetooth->startNotifications(
-      m_webCharacteristic->characteristicInstanceID,
-      new NotificationsCallback(this, resolver));
+
+  // We always check that the device is connected.
+  CHECK(gatt()->connected());
+  gatt()->AddToActiveAlgorithms(resolver);
+
+  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
+  service->RemoteCharacteristicStartNotifications(
+      m_characteristicInstanceId,
+      convertToBaseCallback(
+          WTF::bind(&BluetoothRemoteGATTCharacteristic::NotificationsCallback,
+                    wrapPersistent(this), wrapPersistent(resolver))));
+
   return promise;
 }
 
@@ -372,19 +326,26 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::stopNotifications(
         DOMException::create(NetworkError, kGATTServerNotConnected));
   }
 
-  if (!gatt()->device()->isValidCharacteristic(
-          m_webCharacteristic->characteristicInstanceID)) {
+  if (!gatt()->device()->isValidCharacteristic(m_characteristicInstanceId)) {
     return ScriptPromise::rejectWithDOMException(
         scriptState,
         DOMException::create(InvalidStateError, kInvalidCharacteristic));
   }
 
-  WebBluetooth* webbluetooth =
-      BluetoothSupplement::fromScriptState(scriptState);
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
-  webbluetooth->stopNotifications(m_webCharacteristic->characteristicInstanceID,
-                                  new NotificationsCallback(this, resolver));
+
+  // We always check that the device is connected.
+  CHECK(gatt()->connected());
+  gatt()->AddToActiveAlgorithms(resolver);
+
+  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
+  service->RemoteCharacteristicStopNotifications(
+      m_characteristicInstanceId,
+      convertToBaseCallback(
+          WTF::bind(&BluetoothRemoteGATTCharacteristic::NotificationsCallback,
+                    wrapPersistent(this), wrapPersistent(resolver),
+                    mojom::blink::WebBluetoothResult::SUCCESS)));
   return promise;
 }
 
@@ -392,6 +353,7 @@ DEFINE_TRACE(BluetoothRemoteGATTCharacteristic) {
   visitor->trace(m_service);
   visitor->trace(m_properties);
   visitor->trace(m_value);
+  visitor->trace(m_device);
   EventTargetWithInlineData::trace(visitor);
   SuspendableObject::trace(visitor);
 }