Implement WebBluetooth getDescriptor[s]

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 f19b0a7cf1aa7ce074d8df16928af85f6a7bcd0c..a49eddb1ab58ce885d40fdfb73d75b5833ae6b30 100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
@@ -13,8 +13,10 @@
 #include "core/inspector/ConsoleMessage.h"
 #include "modules/bluetooth/BluetoothCharacteristicProperties.h"
 #include "modules/bluetooth/BluetoothError.h"
+#include "modules/bluetooth/BluetoothRemoteGATTDescriptor.h"
 #include "modules/bluetooth/BluetoothRemoteGATTService.h"
 #include "modules/bluetooth/BluetoothSupplement.h"
+#include "modules/bluetooth/BluetoothUUID.h"
 #include "public/platform/modules/bluetooth/WebBluetooth.h"
 #include <memory>
 
@@ -120,10 +122,122 @@ void BluetoothRemoteGATTCharacteristic::addedEventListener(
   }
 }
 
-class ReadValueCallback : public WebBluetoothReadValueCallbacks {
+class GetDescriptorsCallback : public WebBluetoothGetDescriptorsCallbacks {
  public:
-  ReadValueCallback(BluetoothRemoteGATTCharacteristic* characteristic,
-                    ScriptPromiseResolver* resolver)
+  GetDescriptorsCallback(BluetoothRemoteGATTCharacteristic* characteristic,
+                         mojom::blink::WebBluetoothGATTQueryQuantity quantity,
+                         ScriptPromiseResolver* resolver)
+      : m_characteristic(characteristic),
+        m_quantity(quantity),
+        m_resolver(resolver) {
+    // We always check that the device is connected before constructing this
+    // object.
+    CHECK(m_characteristic->gatt()->connected());

[dcheng, 2016/12/09 08:17:57]

Why is this a CHECK? Either we should handle the error case properly, or this is
attempting to enforce some invariant (so it should be a DCHECK)

[dougt, 2016/12/09 19:20:07]

Hrm.  I think this should be handled properly and we should fail and pass up
that error when the callback object is created.

Also see:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

ortuno, what do you think?

[scheib, 2016/12/10 01:12:59]

DCHECK is right here. (dougt, see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
if you haven't yet). The original patch used CHECK and this wasn't debated, but
the context remains the same. https://codereview.chromium.org/2387293008

+    m_characteristic->gatt()->AddToActiveAlgorithms(m_resolver.get());
+  }
+
+  void onSuccess(const WebVector<WebBluetoothRemoteGATTDescriptorInit*>&
+                     webDescriptors) 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_characteristic->gatt()->RemoveFromActiveAlgorithms(
+            m_resolver.get())) {
+      m_resolver->reject(
+          DOMException::create(NetworkError, kGATTServerDisconnected));
+      return;
+    }
+
+    if (m_quantity == mojom::blink::WebBluetoothGATTQueryQuantity::SINGLE) {
+      DCHECK_EQ(1u, webDescriptors.size());
+      m_resolver->resolve(
+          m_characteristic->gatt()
+              ->device()
+              ->getOrCreateBluetoothRemoteGATTDescriptor(
+                  wrapUnique(webDescriptors[0]), m_characteristic));
+      return;
+    }
+
+    HeapVector<Member<BluetoothRemoteGATTDescriptor>> descriptors;
+    descriptors.reserveInitialCapacity(webDescriptors.size());
+    for (WebBluetoothRemoteGATTDescriptorInit* webDescriptor : webDescriptors) {
+      descriptors.append(m_characteristic->gatt()
+                             ->device()
+                             ->getOrCreateBluetoothRemoteGATTDescriptor(
+                                 wrapUnique(webDescriptor), m_characteristic));

[dcheng, 2016/12/09 08:17:57]

It seems slightly surprising to me that we're transferring (?) ownership out of
a const WebVector<T*>&. If there's ownership transfer involved here, can we just
make the WebVector own unique ptrs and move here?

[dougt, 2016/12/09 19:20:07]

Another question for ortuno or vince -- not sure why we copy them, but most of
the web bluetooth callbacks do just that.

[scheib, 2016/12/10 01:12:59]

Related to the comment in content/renderer/bluetooth/web_bluetooth_impl.cc:
  // TODO(dcheng): This WebVector should use smart pointers.

This will be cleaned up as we go through onion soup and no longer rely on
WebBluetoothGetCharacteristicsCallbacks. This pattern was introduced (from
another confusing one of a unique ptr to a web vector of pointers) in
https://codereview.chromium.org/1861013005. dcheng you're not on that as a
reviewer -- so I'm presuming ortuno added your TODO after discussing with you in
another context.

[ortuno, 2016/12/11 22:12:21]

I first introduced the raw pointers pattern in http://crrev.com/1723723002
because WebPassOwnPtr didn't have a move operator. Then the pattern and TODO
were introduced by dcheng when we got rid of WebPassOwnPtr
http://crrev.com/1865913005. I just copied the code when moving from
bluetooth_dispatcher to web_bluetooth_impl.

If WebVector supports unique_ptrs then I think we could just use unique_ptr and
stop adding this TODO.

+    }
+    m_resolver->resolve(descriptors);
+  }
+
+  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_characteristic->gatt()->RemoveFromActiveAlgorithms(
+            m_resolver.get())) {
+      m_resolver->reject(
+          DOMException::create(NetworkError, kGATTServerDisconnected));
+      return;
+    }
+
+    m_resolver->reject(BluetoothError::take(m_resolver, error));
+  }
+
+ private:
+  Persistent<BluetoothRemoteGATTCharacteristic> m_characteristic;
+  mojom::blink::WebBluetoothGATTQueryQuantity m_quantity;
+  const Persistent<ScriptPromiseResolver> m_resolver;
+};
+
+ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptor(
+    ScriptState* scriptState,
+    const StringOrUnsignedLong& descriptorUUID,
+    ExceptionState& exceptionState) {
+  String descriptor =
+      BluetoothUUID::getDescriptor(descriptorUUID, exceptionState);
+  if (exceptionState.hadException())
+    return exceptionState.reject(scriptState);
+
+  return getDescriptorsImpl(scriptState,
+                            mojom::blink::WebBluetoothGATTQueryQuantity::SINGLE,
+                            descriptor);
+}
+
+ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptors(
+    ScriptState* scriptState,
+    ExceptionState&) {
+  return getDescriptorsImpl(
+      scriptState, mojom::blink::WebBluetoothGATTQueryQuantity::MULTIPLE);
+}
+
+ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptors(
+    ScriptState* scriptState,
+    const StringOrUnsignedLong& descriptorUUID,
+    ExceptionState& exceptionState) {
+  String descriptor =
+      BluetoothUUID::getDescriptor(descriptorUUID, exceptionState);
+  if (exceptionState.hadException())
+    return exceptionState.reject(scriptState);
+
+  return getDescriptorsImpl(
+      scriptState, mojom::blink::WebBluetoothGATTQueryQuantity::MULTIPLE,
+      descriptor);
+}
+
+class CharacteristicReadValueCallback : public WebBluetoothReadValueCallbacks {
+ public:
+  CharacteristicReadValueCallback(
+      BluetoothRemoteGATTCharacteristic* characteristic,
+      ScriptPromiseResolver* resolver)
       : m_characteristic(characteristic), m_resolver(resolver) {
     // We always check that the device is connected before constructing this
     // object.
@@ -193,16 +307,19 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::readValue(
 
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
-  webbluetooth->readValue(m_webCharacteristic->characteristicInstanceID,
-                          new ReadValueCallback(this, resolver));
+  webbluetooth->characteristicReadValue(
+      m_webCharacteristic->characteristicInstanceID,
+      new CharacteristicReadValueCallback(this, resolver));
 
   return promise;
 }
 
-class WriteValueCallback : public WebBluetoothWriteValueCallbacks {
+class CharacteristicWriteValueCallback
+    : public WebBluetoothWriteValueCallbacks {
  public:
-  WriteValueCallback(BluetoothRemoteGATTCharacteristic* characteristic,
-                     ScriptPromiseResolver* resolver)
+  CharacteristicWriteValueCallback(
+      BluetoothRemoteGATTCharacteristic* characteristic,
+      ScriptPromiseResolver* resolver)
       : m_characteristic(characteristic), m_resolver(resolver) {
     // We always check that the device is connected before constructing this
     // object.
@@ -235,7 +352,6 @@ class WriteValueCallback : public WebBluetoothWriteValueCallbacks {
     if (!m_resolver->getExecutionContext() ||
         m_resolver->getExecutionContext()->isContextDestroyed())
       return;
-
     if (!m_characteristic->gatt()->RemoveFromActiveAlgorithms(
             m_resolver.get())) {
       m_resolver->reject(
@@ -286,8 +402,9 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::writeValue(
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
 
   ScriptPromise promise = resolver->promise();
-  webbluetooth->writeValue(m_webCharacteristic->characteristicInstanceID,
-                           valueVector, new WriteValueCallback(this, resolver));
+  webbluetooth->characteristicWriteValue(
+      m_webCharacteristic->characteristicInstanceID, valueVector,
+      new CharacteristicWriteValueCallback(this, resolver));
 
   return promise;
 }
@@ -398,6 +515,44 @@ ScriptPromise BluetoothRemoteGATTCharacteristic::stopNotifications(
   return promise;
 }
 
+ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptorsImpl(
+    ScriptState* scriptState,
+    mojom::blink::WebBluetoothGATTQueryQuantity quantity,
+    const String& descriptor) {
+#if OS(MACOSX)
+  // TODO(crbug.com/624017): Remove when descriptors are implemented on MacOS.
+  return ScriptPromise::rejectWithDOMException(
+      scriptState, DOMException::create(NotSupportedError,
+                                        "getDescriptor[s] is not implemented "
+                                        "yet. See https://goo.gl/J6ASzs"));
+#endif  // OS(MACOSX)
+  WebBluetooth* webbluetooth =
+      BluetoothSupplement::fromScriptState(scriptState);
+
+  if (!gatt()->connected()) {
+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(NetworkError, kGATTServerNotConnected));
+  }
+
+  if (!gatt()->device()->isValidCharacteristic(
+          m_webCharacteristic->characteristicInstanceID)) {
+    return ScriptPromise::rejectWithDOMException(
+        scriptState,
+        DOMException::create(InvalidStateError, kInvalidCharacteristic));
+  }
+
+  ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+  ScriptPromise promise = resolver->promise();
+
+  webbluetooth->getDescriptors(
+      m_webCharacteristic->characteristicInstanceID,
+      static_cast<int32_t>(quantity), descriptor,
+      new GetDescriptorsCallback(this, quantity, resolver));
+
+  return promise;
+}
+
 DEFINE_TRACE(BluetoothRemoteGATTCharacteristic) {
   visitor->trace(m_service);
   visitor->trace(m_properties);