Bug 169759 – [Readable Streams API] Implement ReadableStreamBYOBRequest respond() (readable stream state)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Romain Bellessort]

Implement respond() in readable stream state

[Romain Bellessort]

Created attachment 304655 [details]
Patch

[Build Bot]

Comment on attachment 304655 [details]
Patch

Attachment 304655 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3339459

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 304659 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Romain Bellessort]

Created attachment 304769 [details]
Patch

[youenn fablet]

Comment on attachment 304769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304769&action=review

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:388
> +    return srcBuffer.slice(srcByteOffset, srcByteOffset + srcLength);

This implementation is not shielded.
Also it should probably be implemented in JSC directly.
There is already @structuredCloneArrayBuffer. Maybe we should group these together.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:417
> +    @readableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);

The function names are starting to be very long.
Although it is good to match spec names, this hurts readability.
Maybe we could start some shortening like:
readableByteStreamControllerEnqueueChunkToQueue -> readableByteStreamControllerEnqueueChunk
readableByteStreamControllerCommitPullIntoDescriptor -> readableByteStreamControllerCommitDescriptor
readableByteStreamControllerProcessPullIntoDescriptorsUsingQueue -> readableByteStreamControllerProcessPullDescriptors
...

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:450
> +            @readableByteStreamControllerShiftPendingPullInto(controller);

The naming seems not totally consistent here:
readableByteStreamControllerShiftPendingPullInto
readableByteStreamControllerCommitPullIntoDescriptor

I would rename to something like readableByteStreamControllerShiftPendingDescriptor, readableByteStreamControllerCommitDescriptor

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:503
> +{

Group this function with the other, in a new or another file ideally.

[Romain Bellessort]

Created attachment 305095 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305095 [details]
Patch

Clearing flags on attachment: 305095

Committed r214265: <http://trac.webkit.org/changeset/214265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Romain Bellessort]

(In reply to youenn fablet from comment #5)
> Comment on attachment 304769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304769&action=review
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:388
> > +    return srcBuffer.slice(srcByteOffset, srcByteOffset + srcLength);
> 
> This implementation is not shielded.
> Also it should probably be implemented in JSC directly.
> There is already @structuredCloneArrayBuffer. Maybe we should group these
> together.

Ok I will see how to group them together.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:417
> > +    @readableByteStreamControllerProcessPullIntoDescriptorsUsingQueue(controller);
> 
> The function names are starting to be very long.
> Although it is good to match spec names, this hurts readability.
> Maybe we could start some shortening like:
> readableByteStreamControllerEnqueueChunkToQueue ->
> readableByteStreamControllerEnqueueChunk
> readableByteStreamControllerCommitPullIntoDescriptor ->
> readableByteStreamControllerCommitDescriptor
> readableByteStreamControllerProcessPullIntoDescriptorsUsingQueue ->
> readableByteStreamControllerProcessPullDescriptors
> ...
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:450
> > +            @readableByteStreamControllerShiftPendingPullInto(controller);
> 
> The naming seems not totally consistent here:
> readableByteStreamControllerShiftPendingPullInto
> readableByteStreamControllerCommitPullIntoDescriptor
> 
> I would rename to something like
> readableByteStreamControllerShiftPendingDescriptor,
> readableByteStreamControllerCommitDescriptor
> 

I've updated these names as well as a few other (and I've added comments to indicate the name of corresponding operation in the spec).

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:503
> > +{
> 
> Group this function with the other, in a new or another file ideally.

As this function is called only once by the spec and simply copies data from one buffer to another (no need to detach/transfer/...), I've inlined it.