Add bytes() method for reading bytes into a Uint8Array #370
Conversation
|
Filed Gecko bug. |
|
@martinthomson, @beverloo, looking for interest from a second implementer (and no objections from the third). |
https://bugs.webkit.org/show_bug.cgi?id=274119 rdar://128418858 Reviewed by Youenn Fablet. This implements w3c/FileAPI#198 and w3c/push-api#370 creating parity for these APIs with Fetch's Body. This incorporates the tests from web-platform-tests/wpt#46232 modulo a typo fix. PushMessageData test coverage is done through a local test that would be good to upstream at some point. * LayoutTests/http/wpt/push-api/pushEvent.any.js: (test): * LayoutTests/imported/w3c/web-platform-tests/FileAPI/Blob-methods-from-detached-frame-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/FileAPI/Blob-methods-from-detached-frame.html: * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/Blob-bytes.any-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/Blob-bytes.any.html: Added. * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/Blob-bytes.any.js: Added. (string_appeared_here.promise_test.async const): * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/Blob-bytes.any.worker-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/Blob-bytes.any.worker.html: Added. * LayoutTests/imported/w3c/web-platform-tests/FileAPI/blob/w3c-import.log: * Source/WebCore/Modules/push-api/PushMessageData.cpp: (WebCore::PushMessageData::arrayBuffer): (WebCore::PushMessageData::bytes): * Source/WebCore/Modules/push-api/PushMessageData.h: * Source/WebCore/Modules/push-api/PushMessageData.idl: * Source/WebCore/fileapi/Blob.cpp: (WebCore::Blob::arrayBuffer): (WebCore::Blob::bytes): * Source/WebCore/fileapi/Blob.h: * Source/WebCore/fileapi/Blob.idl: Canonical link: https://commits.webkit.org/279263@main
|
Does this have tests? |
|
I couldn't find any existing tests to extend here, presumably due to #365. So I haven't written any tests for this. Happy to do so if there's a way to do it. |
|
I'm against any significant extension without having a test coverage, but this one is small enough that I guess it's fine given the situation. |
|
We will at least get some limited testing just through the IDL interface. Sadly, most of Push is untested. |
|
Yeah, having some coverage here is in my wishlist. Anyway consider this comment as +1 from Mozilla. |
|
With a +1 from Mozilla and an implementation in Webkit is there anything blocking this from landing? Does it need a statement of non-opposition from Chrome? |
|
We're happy with this, thank you! I'll try to get the change included in Chrome 128. |
That's correct 👍 It's part of WebApp's working mode. We are all about Team Web! 🥰 |
…z,webidl Implements w3c/push-api#370 Differential Revision: https://phabricator.services.mozilla.com/D212319
|
implemented in Gecko, as per referenced Mozilla issue 🥳. |
…z,webidl Implements w3c/push-api#370 Differential Revision: https://phabricator.services.mozilla.com/D212319
…z,webidl Implements w3c/push-api#370 Differential Revision: https://phabricator.services.mozilla.com/D212319
Closes #369
The Fetch API is getting a
Uint8Array-returningbytes()method alongside its existingarrayBuffer()method, following the principle that APIs should generally vend byte buffers asUint8Arrays.This PR makes the same change for
PushMessageData, which has its own distinctarrayBuffermethod.I'm assuming this is uncontroversial given the support from the three major implementations for doing this on
Body, but I can open an issue and solicit explicit support separately if you'd prefer. I'll write tests if I get a signal that this is able to go forward.It's unfortunate that
getKeyandapplicationServerKeyvendArrayBuffers instead ofUint8Arrays, but it's too late to fix those now.The following tasks have been completed:
Implementation commitment:
Preview | Diff