Implement readableByteStreamControllerCallPullIfNeeded()
Created attachment 297462 [details] Patch
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.
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
Created attachment 297516 [details] Patch
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().
Created attachment 297523 [details] Patch
(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.
Comment on attachment 297523 [details] Patch Clearing flags on attachment: 297523 Committed r210027: <http://trac.webkit.org/changeset/210027>
All reviewed patches have been landed. Closing bug.
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.
(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.