Bug 167593 – [Readable Streams API] Implement ReadableByteStreamController pull()

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

[Romain Bellessort]

Implement ReadableByteStreamController pull() internal method

[Romain Bellessort]

Created attachment 300123 [details]
Patch

[Romain Bellessort]

As described in patch, while implementing this function, I found out that @Uint8Array cannot be resolved unless Uint8Array has already been resolved. I will open another bug for this, but for the moment, I have added what seems to be a useless declaration in pull function implementation (const TMP = Uint8Array) so that @Uint8Array can be resolved. If this patch (possibly with improvements) is accepted, removing this line and running the tests added by this patch will be a way to easily reproduce the typed arrays private name resolution bug (and also a way to ensure that proposed fix solves the issue).

[Build Bot]

Comment on attachment 300123 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 300130 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

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

[Romain Bellessort]

Created attachment 300199 [details]
Patch

[Romain Bellessort]

The line "const tmp = Uint8Array" in the builtin function leads to a failure when running tests in debug mode (bad global capture). Therefore, in the new version of the patch, I removed it and set the test expectation to FAIL. Based on this, a patch solving the @Uint8Array issue could be tested by checking that said test no more fails.

[youenn fablet]

Comment on attachment 300199 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:177
> +    if (controller.@totalQueuedBytes === 0 && controller.@closeRequested)

The style is usually !controller.@totalQueuedBytes.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:193
> +        controller.@totalQueuedBytes-= entry.byteLength;

space before -=

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:201
> +        return @Promise.@resolve({value: view, done: false});

I guess we could rewrite this with new @Promise((resolve, reject) => { resolve({value: new @Uint8Array...})).
IIRC though, this is discouraged but do not remember exactly why.

> LayoutTests/streams/readable-byte-stream-controller.js:197
> +        function(err) { assert_object_equals(err, myError); }

Use promise_rejects instead.

> LayoutTests/streams/readable-byte-stream-controller.js:217
> +        function(err) { assert_object_equals(err, myError); }

Ditto.

[Romain Bellessort]

Created attachment 300309 [details]
Patch

[Romain Bellessort]

(In reply to comment #7)
> Comment on attachment 300199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300199&action=review
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:177
> > +    if (controller.@totalQueuedBytes === 0 && controller.@closeRequested)
> 
> The style is usually !controller.@totalQueuedBytes.
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:193
> > +        controller.@totalQueuedBytes-= entry.byteLength;
> 
> space before -=
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:201
> > +        return @Promise.@resolve({value: view, done: false});
> 
> I guess we could rewrite this with new @Promise((resolve, reject) => {
> resolve({value: new @Uint8Array...})).
> IIRC though, this is discouraged but do not remember exactly why.

Ok, since this may be discouraged I have not modified it. Otherwise, all your remarks are addressed in the new patch.

> > LayoutTests/streams/readable-byte-stream-controller.js:197
> > +        function(err) { assert_object_equals(err, myError); }
> 
> Use promise_rejects instead.
> 
> > LayoutTests/streams/readable-byte-stream-controller.js:217
> > +        function(err) { assert_object_equals(err, myError); }
> 
> Ditto.

[WebKit Commit Bot]

Comment on attachment 300309 [details]
Patch

Clearing flags on attachment: 300309

Committed r211484: <http://trac.webkit.org/changeset/211484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.