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

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

Index: third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
diff --git a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
index 84d915fd495db3acdbbf855b6811c19d87763f45..ebfe2cfae6be2f94d27425c4b72f6d296f252800 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
@@ -9,11 +9,10 @@
 #include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/inspector/ConsoleMessage.h"
+#include "modules/bluetooth/Bluetooth.h"
 #include "modules/bluetooth/BluetoothError.h"
 #include "modules/bluetooth/BluetoothRemoteGATTCharacteristic.h"
-#include "modules/bluetooth/BluetoothSupplement.h"
 #include "modules/bluetooth/BluetoothUUID.h"
-#include "public/platform/modules/bluetooth/WebBluetooth.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 #include <utility>
@@ -33,93 +32,69 @@ const char kInvalidService[] =
 }  // namespace
 
 BluetoothRemoteGATTService::BluetoothRemoteGATTService(
-    std::unique_ptr<WebBluetoothRemoteGATTService> webService,
+    const String& serviceInstanceId,
+    const String& uuid,
+    bool isPrimary,
+    const String& deviceInstanceId,
     BluetoothDevice* device)
-    : m_webService(std::move(webService)), m_device(device) {
-  DCHECK(m_webService);
-}
+    : m_serviceInstanceId(serviceInstanceId),
+      m_uuid(uuid),
+      m_isPrimary(isPrimary),
+      m_deviceInstanceId(deviceInstanceId),
+      m_device(device) {}
 
 DEFINE_TRACE(BluetoothRemoteGATTService) {
   visitor->trace(m_device);
 }
 
-// Class that allows us to resolve the promise with a single Characteristic or
-// with a vector owning the characteristics.
-class GetCharacteristicsCallback
-    : public WebBluetoothGetCharacteristicsCallbacks {
- public:
-  GetCharacteristicsCallback(
-      BluetoothRemoteGATTService* service,
-      mojom::blink::WebBluetoothGATTQueryQuantity quantity,
-      ScriptPromiseResolver* resolver)
-      : m_service(service), m_quantity(quantity), m_resolver(resolver) {
-    // We always check that the device is connected before constructing this
-    // object.
-    CHECK(m_service->device()->gatt()->connected());
-    m_service->device()->gatt()->AddToActiveAlgorithms(m_resolver.get());
+// Callback that allows us to resolve the promise with a single Characteristic
+// or with a vector owning the characteristics.
+void BluetoothRemoteGATTService::GetCharacteristicsCallback(
+    const String& serviceInstanceId,
+    mojom::blink::WebBluetoothGATTQueryQuantity quantity,
+    ScriptPromiseResolver* resolver,
+    mojom::blink::WebBluetoothResult result,
+    Optional<Vector<mojom::blink::WebBluetoothRemoteGATTCharacteristicPtr>>
+        characteristics) {
+  if (!resolver->getExecutionContext() ||
+      resolver->getExecutionContext()->isContextDestroyed())
+    return;
+
+  // If the resolver is not in the set of ActiveAlgorithms then the frame
+  // disconnected so we reject.
+  if (!device()->gatt()->RemoveFromActiveAlgorithms(resolver)) {
+    resolver->reject(
+        DOMException::create(NetworkError, kGATTServerDisconnected));
+    return;
   }
 
-  void onSuccess(const WebVector<WebBluetoothRemoteGATTCharacteristicInit*>&
-                     webCharacteristics) override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    // If the resolver is not in the set of ActiveAlgorithms then the frame
-    // disconnected so we reject.
-    if (!m_service->device()->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
-          DOMException::create(NetworkError, kGATTServerDisconnected));
-      return;
-    }
+  if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
+    DCHECK(characteristics);
 
-    if (m_quantity == mojom::blink::WebBluetoothGATTQueryQuantity::SINGLE) {
-      DCHECK_EQ(1u, webCharacteristics.size());
-      m_resolver->resolve(
-          m_service->device()->getOrCreateBluetoothRemoteGATTCharacteristic(
-              m_resolver->getExecutionContext(),
-              WTF::wrapUnique(webCharacteristics[0]), m_service));
+    if (quantity == mojom::blink::WebBluetoothGATTQueryQuantity::SINGLE) {
+      DCHECK_EQ(1u, characteristics->size());
+      resolver->resolve(device()->getOrCreateBluetoothRemoteGATTCharacteristic(

[dcheng, 2016/12/22 08:37:20]

Though this is renderer-side code, it would be nice to use typemap rather than
writing marshalling code like this. Is there a reason we cache characteristics
on a device map?

[dcheng, 2016/12/22 08:37:20]

Btw, I'd consider trying to shorten some of these method names (in a followup):
|device| is already a BluetoothDevice, so the method name can just be
getOrCreateRemoteGATTCharacteristic().

[juncai, 2016/12/22 22:15:40]

Thanks! I filed a bug for this and will do it in a follow-up CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=676703

[juncai, 2016/12/22 22:15:40]

Thanks! I will try this in a follow-up CL, and filed a bug for it:
https://bugs.chromium.org/p/chromium/issues/detail?id=676702

By caching the characteristics on a device map, users of Web Bluetooth can check
which characteristic a device has and use it to construct a GATT service.

[dcheng, 2016/12/27 20:37:37]

I'm just trying to understanding if the caching is a performance optimization or
something else.
If we just always typedmapped, it would reduce the amount of custom marshalling
code here, which is generally considered good.

[juncai, 2016/12/27 21:55:50]

Yes, it is a performance optimization.

+          resolver->getExecutionContext(),
+          characteristics.value()[0]->instance_id, serviceInstanceId,
+          characteristics.value()[0]->uuid,
+          characteristics.value()[0]->properties, this));
       return;
     }
 
-    HeapVector<Member<BluetoothRemoteGATTCharacteristic>> characteristics;
-    characteristics.reserveInitialCapacity(webCharacteristics.size());
-    for (WebBluetoothRemoteGATTCharacteristicInit* webCharacteristic :
-         webCharacteristics) {
-      characteristics.append(
-          m_service->device()->getOrCreateBluetoothRemoteGATTCharacteristic(
-              m_resolver->getExecutionContext(),
-              WTF::wrapUnique(webCharacteristic), m_service));
-    }
-    m_resolver->resolve(characteristics);
-  }
-
-  void onError(
-      int32_t
-          error /* Corresponds to WebBluetoothResult in web_bluetooth.mojom */)
-      override {
-    if (!m_resolver->getExecutionContext() ||
-        m_resolver->getExecutionContext()->isContextDestroyed())
-      return;
-
-    // If the resolver is not in the set of ActiveAlgorithms then the frame
-    // disconnected so we reject.
-    if (!m_service->device()->gatt()->RemoveFromActiveAlgorithms(
-            m_resolver.get())) {
-      m_resolver->reject(
-          DOMException::create(NetworkError, kGATTServerDisconnected));
-      return;
+    HeapVector<Member<BluetoothRemoteGATTCharacteristic>> gattCharacteristics;
+    gattCharacteristics.reserveInitialCapacity(characteristics->size());
+    for (const auto& characteristic : characteristics.value()) {
+      gattCharacteristics.append(
+          device()->getOrCreateBluetoothRemoteGATTCharacteristic(
+              resolver->getExecutionContext(), characteristic->instance_id,
+              serviceInstanceId, characteristic->uuid,
+              characteristic->properties, this));
     }
-
-    m_resolver->reject(BluetoothError::take(m_resolver, error));
+    resolver->resolve(gattCharacteristics);
+  } else {
+    resolver->reject(BluetoothError::take(resolver, result));
   }
-
- private:
-  Persistent<BluetoothRemoteGATTService> m_service;
-  mojom::blink::WebBluetoothGATTQueryQuantity m_quantity;
-  const Persistent<ScriptPromiseResolver> m_resolver;
-};
+}
 
 ScriptPromise BluetoothRemoteGATTService::getCharacteristic(
     ScriptState* scriptState,
@@ -160,26 +135,32 @@ ScriptPromise BluetoothRemoteGATTService::getCharacteristicsImpl(
     ScriptState* scriptState,
     mojom::blink::WebBluetoothGATTQueryQuantity quantity,
     String characteristicsUUID) {

[dcheng, 2016/12/22 08:37:20]

Nit: const String&

[juncai, 2016/12/22 22:15:40]

Done.

+  // We always check that the device is connected.
   if (!device()->gatt()->connected()) {
     return ScriptPromise::rejectWithDOMException(
         scriptState,
         DOMException::create(NetworkError, kGATTServerNotConnected));
   }
 
-  if (!device()->isValidService(m_webService->serviceInstanceID)) {
+  if (!device()->isValidService(m_serviceInstanceId)) {
     return ScriptPromise::rejectWithDOMException(
         scriptState, DOMException::create(InvalidStateError, kInvalidService));
   }
 
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
-
-  WebBluetooth* webbluetooth =
-      BluetoothSupplement::fromScriptState(scriptState);
-  webbluetooth->getCharacteristics(
-      m_webService->serviceInstanceID, static_cast<int32_t>(quantity),
-      characteristicsUUID,
-      new GetCharacteristicsCallback(this, quantity, resolver));
+  device()->gatt()->AddToActiveAlgorithms(resolver);
+
+  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->service();
+  bluetooth::mojom::blink::UUIDPtr uuid = nullptr;
+  if (!characteristicsUUID.isEmpty())
+    uuid = BluetoothUUID::createMojoUuid(characteristicsUUID);

[dcheng, 2016/12/22 08:37:20]

Is it possible to typemap this to String on the Blink side?

[juncai, 2016/12/22 22:15:40]

I am not sure about this, since the uuid is already a typemap to
device:BluetoothUUID:
https://cs.chromium.org/chromium/src/device/bluetooth/public/interfaces/uuid....

[dcheng, 2016/12/27 20:37:37]

Chromium-side and Blink-side typemaps are registered in different places, so
this should be doable.

[juncai, 2016/12/27 21:55:50]

Thanks! I added the typemap for this.

Done.

+  service->RemoteServiceGetCharacteristics(
+      m_serviceInstanceId, quantity, std::move(uuid),
+      convertToBaseCallback(
+          WTF::bind(&BluetoothRemoteGATTService::GetCharacteristicsCallback,
+                    wrapPersistent(this), m_serviceInstanceId, quantity,

[dcheng, 2016/12/22 08:37:20]

wrapWeakPersistent(this)? If this object is going to be destroyed, there's no
point in invoking the callback.

[Reilly Grant (use Gerrit), 2016/12/22 16:23:14]

But if script doesn't keep a reference to this object should its promise really
never get resolved?

[dcheng, 2016/12/27 20:37:37]

OK, fair enough--I'd expect that if you had an otherwise reference-less promise,
the promise should still be resolved.

+                    wrapPersistent(resolver))));
 
   return promise;
 }