Chromium Code Reviews
|
|
Descriptionbluetooth: readValue Blink implementation
Also add a new BluetoothArrayBuffer class so that we can use CallbackPromiseAdapter.
This is the third of three patches to implement readValue:
[1] http://crrev.com/1168113008
[2] http://crrev.com/1149883011
[3] This patch.
BUG=496379
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197084
Patch Set 1 : readValue blink Implementation. #
Total comments: 12
Patch Set 2 : Address jyasskin's comments #
Total comments: 11
Patch Set 3 : Added TODO #Patch Set 4 : Rename BluetoothArrayBuffer to ConverWebVectorToArrayBuffer #
Total comments: 4
Patch Set 5 : Address scheib's comments #Patch Set 6 : Fix Web Exposed Layout Test #
Messages
Total messages: 28 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL
https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:25: assert_unreached("Device went out of range, should fail."); We have ~4 error cases here: Device disappears, device reconfigures to remove the service, device reconfigures to remove the characteristic, and characteristic isn't readable. It's worth testing at least the 3rd and 4th in addition to the 1st. https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:43: var decoder = new TextDecoder(); You should probably specify "utf-8" even though it's the default. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:19: RefPtr<DOMArrayBuffer> domBuffer = DOMArrayBuffer::create( DOMArrayBuffer::create(webVector->data(), webVector->size()). You can static_assert(sizeof(*webVector->data()) == 1, "uint8_t should be a single byte"); https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:18: class BluetoothArrayBuffer { Can you name this "ConvertWebVectorToArrayBuffer"? I don't think "BluetoothArrayBuffer" conveys the type conversion that this is actually doing. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:41: WebBluetooth * webbluetooth = Platform::current()->bluetooth(); Only put a space on one side of the '*'.
Thanks! https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:25: assert_unreached("Device went out of range, should fail."); On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > We have ~4 error cases here: Device disappears, device reconfigures to remove the service, device reconfigures to remove the characteristic, and characteristic isn't readable. It's worth testing at least the 3rd and 4th in addition to the 1st. Added a test for the 4th case. The third is gonna need a decent sized refactoring of the mock provider so I added a TODO. https://codereview.chromium.org/1147243004/diff/60001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:43: var decoder = new TextDecoder(); On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > You should probably specify "utf-8" even though it's the default. Done. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:19: RefPtr<DOMArrayBuffer> domBuffer = DOMArrayBuffer::create( On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > DOMArrayBuffer::create(webVector->data(), webVector->size()). > > You can static_assert(sizeof(*webVector->data()) == 1, "uint8_t should be a single byte"); Done. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:18: class BluetoothArrayBuffer { On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > Can you name this "ConvertWebVectorToArrayBuffer"? I don't think "BluetoothArrayBuffer" conveys the type conversion that this is actually doing. Hmm but converting from WebVector to ArrayBuffer is not really it's purpose. It's main purpose is to provide the interface for CallbackPromiseAdapter. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:41: WebBluetooth * webbluetooth = Platform::current()->bluetooth(); On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > Only put a space on one side of the '*'. Done.
LGTM, but do let Kentaro or another Blink API expert weigh in on the right way to do the ArrayBuffer conversion. https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:18: class BluetoothArrayBuffer { On 2015/06/11 00:10:52, ortuno wrote: > On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > > Can you name this "ConvertWebVectorToArrayBuffer"? I don't think > "BluetoothArrayBuffer" conveys the type conversion that this is actually doing. > > Hmm but converting from WebVector to ArrayBuffer is not really it's purpose. > It's main purpose is to provide the interface for CallbackPromiseAdapter. If it only needed to provide the interface for CallbackPromiseAdapter, you could use any of the other classes that provide the same interface. ;) It has to tell CallbackPromiseAdapter how to glue two specific types together, and it should be named accordingly. Most of the other argument types to CallbackPromiseAdapter and 'Thing' classes that tell it how to convert a WebThing to Thing. This one's not telling it how to convert to a BluetoothArrayBuffer, so I don't think that's a good name. Maybe the *right* answer is to expose this interface on DOMArrayBuffer and then just use that directly? https://codereview.chromium.org/1147243004/diff/80001/LayoutTests/bluetooth/r... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/80001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:46: assert_equals(e.name, 'NetworkError'); And probably a TODO here saying this should be a NotSupportedError.
haraken@chromium.org changed reviewers: + haraken@chromium.org, nhiroki@chromium.org, yhirano@chromium.org
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are structs that are not supposed to be put on the heap (even if it works by accident). I think we should fix the CallbackPromiseAdapter before increasing such manual lifetime management more. nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:10: #include "public/platform/WebVector.h" WebVector will work for your purpose, but wouldn't it be better for content/ to use WebArrayBuffer? As I commented in blink-dev@, you can let content/ use WebArrayBuffer by doing something like: - content/ calls web/ (which can use WebArrayBuffer) - web/ converts WebArrayBuffer into DOMArrayBuffer - web/ calls modules/ (which can use DOMArrayBuffer)
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:10: #include "public/platform/WebVector.h" On 2015/06/11 01:13:13, haraken wrote: > > WebVector will work for your purpose, but wouldn't it be better for content/ to > use WebArrayBuffer? As I commented in blink-dev@, you can let content/ use > WebArrayBuffer by doing something like: > > - content/ calls web/ (which can use WebArrayBuffer) > - web/ converts WebArrayBuffer into DOMArrayBuffer > - web/ calls modules/ (which can use DOMArrayBuffer) I'm not an expert on Promises in Blink, but that's not the flow for any of the uses I can find. Specifically, content isn't calling into modules; modules is calling into content through an interface defined in platform/modules/bluetooth/WebBluetooth.h. The response comes back through the same channel (defined by a CallbackPromiseAdapter created in the call), not through the entirely separate channel a pre-Promises implementation might have used. Do you know of an example that uses Promises in a flow like what you want?
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:10: #include "public/platform/WebVector.h" On 2015/06/11 01:30:36, Jeffrey Yasskin wrote: > On 2015/06/11 01:13:13, haraken wrote: > > > > WebVector will work for your purpose, but wouldn't it be better for content/ > to > > use WebArrayBuffer? As I commented in blink-dev@, you can let content/ use > > WebArrayBuffer by doing something like: > > > > - content/ calls web/ (which can use WebArrayBuffer) > > - web/ converts WebArrayBuffer into DOMArrayBuffer > > - web/ calls modules/ (which can use DOMArrayBuffer) > > I'm not an expert on Promises in Blink, but that's not the flow for any of the > uses I can find. Specifically, content isn't calling into modules; modules is > calling into content through an interface defined in > platform/modules/bluetooth/WebBluetooth.h. The response comes back through the > same channel (defined by a CallbackPromiseAdapter created in the call), not > through the entirely separate channel a pre-Promises implementation might have > used. > > Do you know of an example that uses Promises in a flow like what you want? Ah, thanks; now I understand the flow you want. In that case, it is common to put things in platform/modules/ and call it from content/. Then you cannot use WebArrayBuffer (which is in web/) easily. If you really need to use WebArrayBuffer for some reason, we can do that by implementing a couple of proxy classes, but if WebVector works, that would be the easiest solution.
https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.h (right): https://codereview.chromium.org/1147243004/diff/60001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.h:18: class BluetoothArrayBuffer { On 2015/06/11 at 00:31:37, Jeffrey Yasskin wrote: > On 2015/06/11 00:10:52, ortuno wrote: > > On 2015/06/10 at 21:12:25, Jeffrey Yasskin wrote: > > > Can you name this "ConvertWebVectorToArrayBuffer"? I don't think > > "BluetoothArrayBuffer" conveys the type conversion that this is actually doing. > > > > Hmm but converting from WebVector to ArrayBuffer is not really it's purpose. > > It's main purpose is to provide the interface for CallbackPromiseAdapter. > > If it only needed to provide the interface for CallbackPromiseAdapter, you could use any of the other classes that provide the same interface. ;) It has to tell CallbackPromiseAdapter how to glue two specific types together, and it should be named accordingly. > > Most of the other argument types to CallbackPromiseAdapter and 'Thing' classes that tell it how to convert a WebThing to Thing. This one's not telling it how to convert to a BluetoothArrayBuffer, so I don't think that's a good name. > Ah ok. That makes sense. > Maybe the *right* answer is to expose this interface on DOMArrayBuffer and then just use that directly? You mean the CallbackPromiseAdapter interface? I haven't seen any platform/ objects implement the CallbackPromiseAdapter interface. https://codereview.chromium.org/1147243004/diff/80001/LayoutTests/bluetooth/r... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/80001/LayoutTests/bluetooth/r... LayoutTests/bluetooth/readValue.html:46: assert_equals(e.name, 'NetworkError'); On 2015/06/11 at 00:31:37, Jeffrey Yasskin wrote: > And probably a TODO here saying this should be a NotSupportedError. Done. https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); On 2015/06/11 at 01:13:13, haraken wrote: > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are structs that are not supposed to be put on the heap (even if it works by accident). > Both CallbackPromiseAdapter and WebCallbacks require a T*. So I think we're gonna have to put the vector in the heap either way. > I think we should fix the CallbackPromiseAdapter before increasing such manual lifetime management more. > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter?
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); On 2015/06/11 02:56:33, ortuno wrote: > On 2015/06/11 at 01:13:13, haraken wrote: > > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are > structs that are not supposed to be put on the heap (even if it works by > accident). > > > > Both CallbackPromiseAdapter and WebCallbacks require a T*. So I think we're > gonna have to put the vector in the heap either way. > > > I think we should fix the CallbackPromiseAdapter before increasing such manual > lifetime management more. > > > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? > Yeah. BTW, why do we need this OwnPtr? Can we just use webVectorRawPointer->data() below?
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); On 2015/06/11 01:13:13, haraken wrote: > > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are > structs that are not supposed to be put on the heap (even if it works by > accident). > > I think we should fix the CallbackPromiseAdapter before increasing such manual > lifetime management more. > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? Updated my current status in the issue: https://code.google.com/p/chromium/issues/detail?id=493531#c7 (In summary, I need somewhat more time to complete..., sorry)
On 2015/06/11 04:01:01, nhiroki wrote: > https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... > File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): > > https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... > Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> > webVector = adoptPtr(webVectorRawPointer); > On 2015/06/11 01:13:13, haraken wrote: > > > > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are > > structs that are not supposed to be put on the heap (even if it works by > > accident). > > > > I think we should fix the CallbackPromiseAdapter before increasing such manual > > lifetime management more. > > > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? > > Updated my current status in the issue: > https://code.google.com/p/chromium/issues/detail?id=493531#c7 > > (In summary, I need somewhat more time to complete..., sorry) Thanks for working on it!
https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); On 2015/06/11 at 03:34:26, haraken wrote: > On 2015/06/11 02:56:33, ortuno wrote: > > On 2015/06/11 at 01:13:13, haraken wrote: > > > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives are > > structs that are not supposed to be put on the heap (even if it works by > > accident). > > > > > > > Both CallbackPromiseAdapter and WebCallbacks require a T*. So I think we're > > gonna have to put the vector in the heap either way. > > > > > I think we should fix the CallbackPromiseAdapter before increasing such manual > > lifetime management more. > > > > > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? > > > > Yeah. BTW, why do we need this OwnPtr? Can we just use webVectorRawPointer->data() below? I could but then I would have to manually delete webVectorRawPointer, right?
scheib@chromium.org changed reviewers: + scheib@chromium.org
https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... LayoutTests/bluetooth/readValue.html:51: }, 'Device goes out of range. Reject with NetworkError.'); Update test description. https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... LayoutTests/bluetooth/readValue.html:53: // TODO(ortuno): Add a test for when a characterstics gets removed. Create a crbug.com issue so we can track known TODOs.
@scheib: Thanks. Ready for review again. https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... File LayoutTests/bluetooth/readValue.html (right): https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... LayoutTests/bluetooth/readValue.html:51: }, 'Device goes out of range. Reject with NetworkError.'); On 2015/06/11 at 20:19:13, scheib wrote: > Update test description. Done. https://codereview.chromium.org/1147243004/diff/120001/LayoutTests/bluetooth/... LayoutTests/bluetooth/readValue.html:53: // TODO(ortuno): Add a test for when a characterstics gets removed. On 2015/06/11 at 20:19:13, scheib wrote: > Create a crbug.com issue so we can track known TODOs. Done.
LGTM
LGTM https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... File Source/modules/bluetooth/BluetoothArrayBuffer.cpp (right): https://codereview.chromium.org/1147243004/diff/80001/Source/modules/bluetoot... Source/modules/bluetooth/BluetoothArrayBuffer.cpp:14: OwnPtr<WebVector<uint8_t>> webVector = adoptPtr(webVectorRawPointer); On 2015/06/11 16:24:40, ortuno wrote: > On 2015/06/11 at 03:34:26, haraken wrote: > > On 2015/06/11 02:56:33, ortuno wrote: > > > On 2015/06/11 at 01:13:13, haraken wrote: > > > > OwnPtr<WebVector> looks really weird. WebVector and other Web primitives > are > > > structs that are not supposed to be put on the heap (even if it works by > > > accident). > > > > > > > > > > Both CallbackPromiseAdapter and WebCallbacks require a T*. So I think we're > > > gonna have to put the vector in the heap either way. > > > > > > > I think we should fix the CallbackPromiseAdapter before increasing such > manual > > > lifetime management more. > > > > > > > > nhiroki, yhirano: Any progress/plan to fix CallbackPromiseAdapter? > > > > > > > Yeah. BTW, why do we need this OwnPtr? Can we just use > webVectorRawPointer->data() below? > > I could but then I would have to manually delete webVectorRawPointer, right? Ah, understood. I hope that the manual lifetime management will be fixed soon.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, haraken@chromium.org, scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1147243004/#ps160001 (title: "Fix Web Exposed Layout Test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1147243004/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197084 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
