Bug 167593 - [Readable Streams API] Implement ReadableByteStreamController pull()
Summary: [Readable Streams API] Implement ReadableByteStreamController pull()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Romain Bellessort
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-30 09:50 PST by Romain Bellessort
Modified: 2017-02-01 08:19 PST (History)
4 users (show)

See Also:


Attachments
Patch (11.29 KB, patch)
2017-01-30 10:49 PST, Romain Bellessort
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (382.60 KB, application/zip)
2017-01-30 11:46 PST, Build Bot
no flags Details
Patch (10.98 KB, patch)
2017-01-31 02:20 PST, Romain Bellessort
no flags Details | Formatted Diff | Diff
Patch (10.98 KB, patch)
2017-02-01 02:06 PST, Romain Bellessort
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Romain Bellessort 2017-01-30 09:50:25 PST
Implement ReadableByteStreamController pull() internal method
Comment 1 Romain Bellessort 2017-01-30 10:49:33 PST
Created attachment 300123 [details]
Patch
Comment 2 Romain Bellessort 2017-01-30 11:06:37 PST
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).
Comment 3 Build Bot 2017-01-30 11:46:22 PST
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.
Comment 4 Build Bot 2017-01-30 11:46:26 PST
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
Comment 5 Romain Bellessort 2017-01-31 02:20:44 PST
Created attachment 300199 [details]
Patch
Comment 6 Romain Bellessort 2017-01-31 03:29:53 PST
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.
Comment 7 youenn fablet 2017-01-31 22:00:59 PST
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.
Comment 8 Romain Bellessort 2017-02-01 02:06:02 PST
Created attachment 300309 [details]
Patch
Comment 9 Romain Bellessort 2017-02-01 05:54:18 PST
(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.
Comment 10 WebKit Commit Bot 2017-02-01 08:19:17 PST
Comment on attachment 300309 [details]
Patch

Clearing flags on attachment: 300309

Committed r211484: <http://trac.webkit.org/changeset/211484>
Comment 11 WebKit Commit Bot 2017-02-01 08:19:21 PST
All reviewed patches have been landed.  Closing bug.