Bug 166022 – [Readable Streams API] Implement readableByteStreamControllerCallPullIfNeeded()

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

[Romain Bellessort]

Implement readableByteStreamControllerCallPullIfNeeded()

[Romain Bellessort]

Created attachment 297462 [details]
Patch

[Build Bot]

Comment on attachment 297462 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 297470 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Romain Bellessort]

Created attachment 297516 [details]
Patch

[youenn fablet]

Comment on attachment 297516 [details]
Patch

Looks ok to me.
I wonder whether we could not add some more tests even though the implementation is not yet finished?

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

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:166
> +    return true;

Could be made as a one liner

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:176
> +        return false;

Ditto.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:192
> +        @readableByteStreamControllerGetDesiredSize(controller) > 0)) {

These 3 lines if are not very readable, maybe split them in 3 different if?

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:201
> +    @promiseInvokeOrNoop(controller.@underlyingByteSource, "pull", [controller]).@then(function() {

Above, you use arrow functions and there you use function.
Is there a reason for that?

> LayoutTests/streams/readable-byte-stream-controller.js:239
> +    }), 200);

There were some changes due to timeout values in github streams whatwg wpt tests. 
This sounds ok to me but you might want to check whether this is in line with these chnanges.

Here also, you can use promise_test by returning a promise created by the script using "new Promise".
You will need to call the resolve callback instead of testPull.done().

[Romain Bellessort]

Created attachment 297523 [details]
Patch

[Romain Bellessort]

(In reply to comment #5)
> Comment on attachment 297516 [details]
> Patch
> 
> Looks ok to me.
> I wonder whether we could not add some more tests even though the
> implementation is not yet finished?

I think that most of the new code that can be tested is tested by the 3 new tests, but I'll add more tests as implementation progresses.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297516&action=review
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:166
> > +    return true;
> 
> Could be made as a one liner
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:176
> > +        return false;
> 
> Ditto.

Done for both.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:192
> > +        @readableByteStreamControllerGetDesiredSize(controller) > 0)) {
> 
> These 3 lines if are not very readable, maybe split them in 3 different if?

Done.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:201
> > +    @promiseInvokeOrNoop(controller.@underlyingByteSource, "pull", [controller]).@then(function() {
> 
> Above, you use arrow functions and there you use function.
> Is there a reason for that?

No reason, hence I replaced by arrow function.

> > LayoutTests/streams/readable-byte-stream-controller.js:239
> > +    }), 200);
> 
> There were some changes due to timeout values in github streams whatwg wpt
> tests. 
> This sounds ok to me but you might want to check whether this is in line
> with these chnanges.

I'm not sure about the changes you mention. For the moment I'll keep the tests as they are, and I may further investigate if they are uploaded to WPT.

> Here also, you can use promise_test by returning a promise created by the
> script using "new Promise".
> You will need to call the resolve callback instead of testPull.done().

Thanks, I made the modification and code is cleaner.

[WebKit Commit Bot]

Comment on attachment 297523 [details]
Patch

Clearing flags on attachment: 297523

Committed r210027: <http://trac.webkit.org/changeset/210027>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 297523 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:185
> +    if (!@readableStreamHasDefaultReader(stream) || stream.@reader.@readRequests <= 0)
> +        if (!@readableStreamHasBYOBReader(stream) || stream.@reader.@readIntoRequests <= 0)
> +            if (@readableByteStreamControllerGetDesiredSize(controller) <= 0)
> +                return;

Style: WebKit style is to use braces for multi-line if statements.
Style: This could could really use some newline in between the if blocks / blocks of code to make this easier for humans to read.

Why do `readableStreamHasDefaultReader` and `readableStreamHasBYOBReader` both check `stream.@reader !== undefined` but if that fails we could end up here checking for properties on undefined, such as `stream.@reader.@readRequests`.

It seems like we could drop the undefined check in the helper functions or these checks need to be made safer.

[Romain Bellessort]

(In reply to comment #10)
> Comment on attachment 297523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297523&action=review
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:185
> > +    if (!@readableStreamHasDefaultReader(stream) || stream.@reader.@readRequests <= 0)
> > +        if (!@readableStreamHasBYOBReader(stream) || stream.@reader.@readIntoRequests <= 0)
> > +            if (@readableByteStreamControllerGetDesiredSize(controller) <= 0)
> > +                return;
> 
> Style: WebKit style is to use braces for multi-line if statements.
> Style: This could could really use some newline in between the if blocks /
> blocks of code to make this easier for humans to read.
> 
> Why do `readableStreamHasDefaultReader` and `readableStreamHasBYOBReader`
> both check `stream.@reader !== undefined` but if that fails we could end up
> here checking for properties on undefined, such as
> `stream.@reader.@readRequests`.
> 
> It seems like we could drop the undefined check in the helper functions or
> these checks need to be made safer.

My bad, I rewrote this test too quickly. I created a follow up bug (https://bugs.webkit.org/show_bug.cgi?id=166312) to fix this test and make it more readable. The spec defines a readableByteStreamShouldCallPull function where these tests are performed in a cleaner way. My initial intent was to integrate this function directly in readableByteStreamCallPullIfNeeded, but it seems better to keep two distinct functions for readability.