| Summary: | [Streams API] Align internal structure of ReadableStream with spec | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Romain Bellessort <romain.wkt> | ||||||||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Romain Bellessort <romain.wkt> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, calvaris, commit-queue, rniwa, youennf | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Romain Bellessort
2016-07-28 09:59:41 PDT
Created attachment 284789 [details]
Patch
Various properties and behaviors have been updated, especially in ReadableStreamInternals.js . Other changes will have to be done in order to fully align with spec, but my intent with this patch was to make the minimal changes in order to adopt the new internal structure and to pass tests. Comment on attachment 284789 [details] Patch Attachment 284789 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1768635 Number of test failures exceeded the failure limit. Created attachment 284794 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 284789 [details] Patch Attachment 284789 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1768659 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/fetch/api/response/response-consume.html Created attachment 284799 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 284789 [details] Patch Attachment 284789 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1768670 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/fetch/api/response/response-consume.html Created attachment 284800 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 284789 [details] Patch Attachment 284789 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1768665 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/fetch/api/response/response-consume.html Created attachment 284801 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 285116 [details]
Patch
Comment on attachment 285116 [details] Patch Attachment 285116 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1799422 Number of test failures exceeded the failure limit. Created attachment 285118 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 285211 [details]
Patch
Comment on attachment 285211 [details] Patch Attachment 285211 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1803437 Number of test failures exceeded the failure limit. Created attachment 285214 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 285216 [details]
Patch
(In reply to comment #2) > Various properties and behaviors have been updated, especially in > ReadableStreamInternals.js . Other changes will have to be done in order to > fully align with spec, but my intent with this patch was to make the minimal > changes in order to adopt the new internal structure and to pass tests. All the tests are now passing. Comment on attachment 285216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285216&action=review I haven't finished this review yet, will do it tomorrow. > Source/WebCore/ChangeLog:15 > + No change in functionality. There's a test that changes expectations so this is covered by that, mention it here, please. > Source/WebCore/ChangeLog:43 > + * Modules/fetch/FetchResponse.js: > + (initializeFetchResponse): > + * Modules/streams/ReadableStream.js: > + (initializeReadableStream): > + * Modules/streams/ReadableStreamDefaultController.js: > + (enqueue): > + (error): > + (close): > + (desiredSize): > + * Modules/streams/ReadableStreamInternals.js: > + (privateInitializeReadableStreamDefaultController): > + (readableStreamDefaultControllerError): > + (teeReadableStream): > + (teeReadableStreamPullFunction): > + (isReadableStream): > + (isReadableStreamDefaultReader): > + (isReadableStreamDefaultController): > + (errorReadableStream): > + (requestReadableStreamPull): > + (readableStreamDefaultControllerGetDesiredSize): > + (cancelReadableStream): > + (readableStreamDefaultControllerClose): > + (closeReadableStream): > + (enqueueInReadableStream): > + (readFromReadableStreamDefaultReader): > + (teeReadableStreamBranch2CancelFunction): > + * bindings/js/WebCoreBuiltinNames.h: I think you could be a bit more verbose here. > Source/WebCore/Modules/streams/ReadableStream.js:54 > + // TODO Implement support of ReadableByteStreamController There are no TODOs in WebKit, only FIXMEs. You need a semicolon after the FIXME and finish the comment with a period. https://webkit.org/code-style-guidelines/#comments > Source/WebCore/Modules/streams/ReadableStream.js:64 > + else { > + throw new @RangeError("Invalid type for underlying source") > + } You don't need there brackets here. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:76 > + this.@strategySize = normalizedStrategy.size; > + this.@strategyHWM = normalizedStrategy.highWaterMark; I would suggest to use an strategy object here as we had in ReadableStream.js:56. We would be saving adding the two new ids that the BuiltinNames... > Source/WebCore/Modules/streams/ReadableStreamInternals.js:82 > + @assert(!controller.@pulling); > + @assert(!controller.@pullAgain) Where do these asserts come from? > Source/WebCore/Modules/streams/ReadableStreamInternals.js:214 > + // Spec tells to return true only if stream has a readableStreamController internal slot > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > + // Therefore, the check is made based on instanceof These sentences lack a period at their ends > Source/WebCore/Modules/streams/ReadableStreamInternals.js:230 > + // Spec tells to return true only if reader has a readRequests internal slot > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > + // Since readRequests is initialized with an empty array, the following test is ok Sentences with periods, please. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:247 > + // Spec tells to return true only if controller has an underlyingSource internal slot > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > + // underlyingSource is obtained in ReadableStream constructor: if undefined, it is set > + // to an empty object. Therefore, following test is ok Sentences with periods, please. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:254 > + You don't need this extra line > Source/WebCore/Modules/streams/ReadableStreamInternals.js:276 > + // TODO Implement ReadableStreamBYOBReader No TODOs and proper formatting. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:345 > + // TODO Fix below, which is a temporary solution to the case where controller is undefined > + // This issue is due to the fact that in previous version of the spec, cancel was associated > + // to underlyingSource, whereas in new version it is associated to controller. As this patch > + // does not yet fully implement the new version, this solution is used. No TODOs and proper formatting. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:-294 > - I think removing this line is putting a function just after the other without space, better to have one. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:-344 > - I didn't comment in some other empty lines cause that's programmer's style, but I would recommend to leave this line. Thanks for these first comments, I've prepared the corresponding changes. Please find additional responses below for changes which are not related to comments/coding style. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:76 > > + this.@strategySize = normalizedStrategy.size; > > + this.@strategyHWM = normalizedStrategy.highWaterMark; > > I would suggest to use an strategy object here as we had in > ReadableStream.js:56. We would be saving adding the two new ids that the > BuiltinNames... Ok, I wanted to align with spec but now I understand the benefits of not having two new ids in BuiltinNames. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:82 > > + @assert(!controller.@pulling); > > + @assert(!controller.@pullAgain) > > Where do these asserts come from? I mixed ReadableByteStreamController behaviour and ReadableStreamDefaultController while checking the spec too quickly, sorry for that. Comment on attachment 285216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285216&action=review > Source/WebCore/Modules/streams/ReadableStreamInternals.js:218 > - return @isObject(stream) && !!stream.@underlyingSource; > + if (!@isObject(stream)) > + return false; > + > + // Spec tells to return true only if stream has a readableStreamController internal slot > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > + // Therefore, the check is made based on instanceof > + if (!(stream instanceof @ReadableStream)) > + return false; > + > + return true; The spec does not say that you have to check if the elements is actually an instance of ReadableStream, it has to check it's an object and it has a readableStreamController slot. I strongly think we should stick to the spec and keep the original implementation with the check for the existance of readableStreamController instead of underlyingSource. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:234 > - // To reset @ownerReadableStream it must be set to null instead of undefined because there is no way to distinguish > - // between a non-existent slot and an slot set to undefined. > - return @isObject(reader) && reader.@ownerReadableStream !== @undefined; > + if (!@isObject(reader)) > + return false; > + > + // Spec tells to return true only if reader has a readRequests internal slot > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > + // Since readRequests is initialized with an empty array, the following test is ok > + if (reader.@readRequests === @undefined) > + return false; > + > + return true; Same here. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:396 > - @requestReadableStreamPull(stream); > + @requestReadableStreamPull(controller); I fail to see where this is now on the spec, could it have been removed at some point? > Source/WebCore/Modules/streams/ReadableStreamInternals.js:405 > + chunkSize = @Number(controller.@strategySize(chunk)); > + if (!@isFinite(chunkSize) || chunkSize < 0) > throw new @RangeError("Chunk size is not valid"); These checks are not here in the spec and are redundant cause they are done in enqueueValueWithSize. Not even the @Number conversion is needed. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:425 > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback) > - stream.@underlyingSource.@firstReadCallback(); > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback) > + stream.@readableStreamController.@underlyingSource.@firstReadCallback(); This part does not seem to be in the spec either. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:444 > - if (stream.@queue.content.length) { > - const chunk = @dequeueValue(stream.@queue); > - if (stream.@closeRequested && stream.@queue.content.length === 0) > - @finishClosingReadableStream(stream); > + if (stream.@readableStreamController.@queue.content.length) { > + const chunk = @dequeueValue(stream.@readableStreamController.@queue); > + if (stream.@readableStreamController.@closeRequested && stream.@readableStreamController.@queue.content.length === 0) > + @closeReadableStream(stream); > else > - @requestReadableStreamPull(stream); > + @requestReadableStreamPull(stream.@readableStreamController); > return @Promise.@resolve({value: chunk, done: false}); > } > const readPromiseCapability = @newPromiseCapability(@Promise); > reader.@readRequests.@push(readPromiseCapability); > - @requestReadableStreamPull(stream); > + @requestReadableStreamPull(stream.@readableStreamController); > return readPromiseCapability.@promise; This seems to be step 7 of the spec. I don't know if this par of the spec changed or not, but it looks like it can be reworked into calling a function that should already exist... > Source/WebCore/bindings/js/WebCoreBuiltinNames.h:88 > + macro(strategyHWM) \ > + macro(strategySize) \ If we use the strategy these are not needed. Comment on attachment 285216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285216&action=review This was one of my most common mistakes :) > Source/WebCore/Modules/streams/ReadableStreamInternals.js:92 > +function readableStreamDefaultControllerError(controller, error) { This function misses the "use strict"; > Source/WebCore/Modules/streams/ReadableStreamInternals.js:356 > + "user strict"; "use strict"; Comment on attachment 285216 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285216&action=review > Source/WebCore/Modules/streams/ReadableStreamDefaultController.js:55 > + @errorReadableStream(this.@controlledReadableStream, error); This change is probably not needed. >> Source/WebCore/Modules/streams/ReadableStreamInternals.js:218 >> + return true; > > The spec does not say that you have to check if the elements is actually an instance of ReadableStream, it has to check it's an object and it has a readableStreamController slot. I strongly think we should stick to the spec and keep the original implementation with the check for the existance of readableStreamController instead of underlyingSource. I am not sure which option is best here. Either way is fine since they return the same result. On the instanceof bonus side, we may not even check for @isObject which would make the function a tad smaller and faster. On the instanceof downside, @isPromise is doing the same as we were doing before, so maybe we should keep being consistent here. This choice allows creating Internal promises the have a different type but can still use Promise prototype functions. I do not see reasons for us to do other-type streams but who knows in the future. Created attachment 285330 [details]
Patch
(In reply to comment #21) > Comment on attachment 285216 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285216&action=review > > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:218 > > - return @isObject(stream) && !!stream.@underlyingSource; > > + if (!@isObject(stream)) > > + return false; > > + > > + // Spec tells to return true only if stream has a readableStreamController internal slot > > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > > + // Therefore, the check is made based on instanceof > > + if (!(stream instanceof @ReadableStream)) > > + return false; > > + > > + return true; > > The spec does not say that you have to check if the elements is actually an > instance of ReadableStream, it has to check it's an object and it has a > readableStreamController slot. I strongly think we should stick to the spec > and keep the original implementation with the check for the existance of > readableStreamController instead of underlyingSource. The issue here is that readableStreamController is initially undefined and we can't check its presence. That's why I chose to base the check on instanceof. Based on discussions, I removed instanceof check and opted for initializing readableStreamController with null value. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:234 > > - // To reset @ownerReadableStream it must be set to null instead of undefined because there is no way to distinguish > > - // between a non-existent slot and an slot set to undefined. > > - return @isObject(reader) && reader.@ownerReadableStream !== @undefined; > > + if (!@isObject(reader)) > > + return false; > > + > > + // Spec tells to return true only if reader has a readRequests internal slot > > + // However, since it is a private slot, it cannot be checked using hasOwnProperty() > > + // Since readRequests is initialized with an empty array, the following test is ok > > + if (reader.@readRequests === @undefined) > > + return false; > > + > > + return true; > > Same here. Here the right internal slot is checked according to spec. I updated this in new patch to have a single line test, as in previous version. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:396 > > - @requestReadableStreamPull(stream); > > + @requestReadableStreamPull(controller); > > I fail to see where this is now on the spec, could it have been removed at > some point? I think this is step 6 of ReadableStreamDefaultControllerEnqueue (called ReadableStreamDefaultControllerCallPullIfNeeded in latest spec, but I haven’t updated such names for the moment). > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:405 > > + chunkSize = @Number(controller.@strategySize(chunk)); > > + if (!@isFinite(chunkSize) || chunkSize < 0) > > throw new @RangeError("Chunk size is not valid"); > > These checks are not here in the spec and are redundant cause they are done > in enqueueValueWithSize. Not even the @Number conversion is needed. Right, I removed them. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:425 > > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback) > > - stream.@underlyingSource.@firstReadCallback(); > > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback) > > + stream.@readableStreamController.@underlyingSource.@firstReadCallback(); > > This part does not seem to be in the spec either. Even though not in the spec, there is a comment above that suggest it is useful: "Native source may want to start enqueueing at the time of the first read request". I tried to remove code but it led to unexpected failures with Fetch API. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:444 > > - if (stream.@queue.content.length) { > > - const chunk = @dequeueValue(stream.@queue); > > - if (stream.@closeRequested && stream.@queue.content.length === 0) > > - @finishClosingReadableStream(stream); > > + if (stream.@readableStreamController.@queue.content.length) { > > + const chunk = @dequeueValue(stream.@readableStreamController.@queue); > > + if (stream.@readableStreamController.@closeRequested && stream.@readableStreamController.@queue.content.length === 0) > > + @closeReadableStream(stream); > > else > > - @requestReadableStreamPull(stream); > > + @requestReadableStreamPull(stream.@readableStreamController); > > return @Promise.@resolve({value: chunk, done: false}); > > } > > const readPromiseCapability = @newPromiseCapability(@Promise); > > reader.@readRequests.@push(readPromiseCapability); > > - @requestReadableStreamPull(stream); > > + @requestReadableStreamPull(stream.@readableStreamController); > > return readPromiseCapability.@promise; > > This seems to be step 7 of the spec. I don't know if this par of the spec > changed or not, but it looks like it can be reworked into calling a function > that should already exist... Spec moved this code to a new function, which I have added in new patch. The new function is a pull function internal to ReadableStreamDefaultController. I also added the readableStreamAddReadRequest described by the spec and called in this pull function. > > Source/WebCore/bindings/js/WebCoreBuiltinNames.h:88 > > + macro(strategyHWM) \ > > + macro(strategySize) \ > > If we use the strategy these are not needed. I went back to using strategy. (In reply to comment #23) > Comment on attachment 285216 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285216&action=review > > > Source/WebCore/Modules/streams/ReadableStreamDefaultController.js:55 > > + @errorReadableStream(this.@controlledReadableStream, error); > > This change is probably not needed. Thanks, I fixed it. Comment on attachment 285330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285330&action=review > Source/WebCore/ChangeLog:21 > + (initializeFetchResponse): replaced reference to underlyingSource as no Sentences here should begin with capitals. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:108 > + "user strict"; "use strict"; > Source/WebCore/Modules/streams/ReadableStreamInternals.js:395 > - @requestReadableStreamPull(stream); > + @requestReadableStreamPull(controller); This line seems to be out of the spec. > Source/WebCore/Modules/streams/ReadableStreamInternals.js:422 > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback) > - stream.@underlyingSource.@firstReadCallback(); > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback) > + stream.@readableStreamController.@underlyingSource.@firstReadCallback(); I understand that the chunk below corresponds to the @pull, but I can't find this on the spec yet. Created attachment 285336 [details]
Patch
(In reply to comment #26) > Comment on attachment 285330 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285330&action=review > > > Source/WebCore/ChangeLog:21 > > + (initializeFetchResponse): replaced reference to underlyingSource as no > > Sentences here should begin with capitals. Fixed. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:108 > > + "user strict"; > > "use strict"; > > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:395 > > - @requestReadableStreamPull(stream); > > + @requestReadableStreamPull(controller); > > This line seems to be out of the spec. The reason for this line is not clear to me, but if I remove it, some W3C imported Streams tests fail unexpectedly. Therefore I suggest keeping it for the moment and investigating later on. > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:422 > > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback) > > - stream.@underlyingSource.@firstReadCallback(); > > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback) > > + stream.@readableStreamController.@underlyingSource.@firstReadCallback(); > > I understand that the chunk below corresponds to the @pull, but I can't find > this on the spec yet. As I said in previous comment, this code is not in spec, but when removed, some Fetch API tests are failing unexpectedly. It is associated to the following comment: "Native source may want to start enqueueing at the time of the first read request". This code was there prior to my patch and I only updated it in order to keep tests ok. > > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:422
> > > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback)
> > > - stream.@underlyingSource.@firstReadCallback();
> > > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback)
> > > + stream.@readableStreamController.@underlyingSource.@firstReadCallback();
> >
> > I understand that the chunk below corresponds to the @pull, but I can't find
> > this on the spec yet.
>
> As I said in previous comment, this code is not in spec, but when removed,
> some Fetch API tests are failing unexpectedly. It is associated to the
> following comment: "Native source may want to start enqueueing at the time
> of the first read request". This code was there prior to my patch and I only
> updated it in order to keep tests ok.
This is a private callback used when creating a ReadableStream from a FetchResponse being loaded.
It allows the source to not pull immediately when "start" is called but only at the first reader.read() call.
It was necessary at some point so that the stream could not be read but fetch data be consumed with text/json/arrayBuffer accessors.
I think that we could probably remove this callback.
But this allows buffering the data before the user actually makes a read() call, which is a kind of optimisation.
In this bug, it might be best to keep it like it is now and address this in another bug. (In reply to comment #25) > I think this is step 6 of ReadableStreamDefaultControllerEnqueue (called > ReadableStreamDefaultControllerCallPullIfNeeded in latest spec, but I > haven’t updated such names for the moment). Yes, definitely. I was misreading the spec and forgetting that the step 6 was out of the step 5. My bad, sorry :) > > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:425 > > > - if (!stream.@disturbed && stream.@underlyingSource.@firstReadCallback) > > > - stream.@underlyingSource.@firstReadCallback(); > > > + if (!stream.@disturbed && stream.@readableStreamController.@underlyingSource.@firstReadCallback) > > > + stream.@readableStreamController.@underlyingSource.@firstReadCallback(); > > > > This part does not seem to be in the spec either. > > Even though not in the spec, there is a comment above that suggest it is > useful: "Native source may want to start enqueueing at the time of the first > read request". I tried to remove code but it led to unexpected failures with > Fetch API. Ok, I see Youenn's point. Let's keep it, but please write Youenn's clarification in a comment to avoid this doubts in the future. Comment on attachment 285336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285336&action=review Sorry that I missed some of the style issues before. These things can be fixed at landing time > Source/WebCore/Modules/streams/ReadableStream.js:58 > + } > + else if (type === @undefined) { } else if... > Source/WebCore/Modules/streams/ReadableStream.js:63 > + } > + else } else > Source/WebCore/Modules/streams/ReadableStreamInternals.js:86 > + this.@pull = function() > + { this.@pull = function () { > Source/WebCore/Modules/streams/ReadableStreamInternals.js:273 > + } > + else { } else > Source/WebCore/Modules/streams/ReadableStreamInternals.js:420 > // Native sources may want to start enqueueing at the time of the first read request. I see there's already a comment, so disregard my previous comment about adding anything here. Created attachment 287511 [details]
Patch
Comment on attachment 287511 [details] Patch Rejecting attachment 287511 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 287511, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 1 succeeded at 1 with fuzz 3. patching file LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/general.https-expected.txt Hunk #1 FAILED at 1. Hunk #2 FAILED at 38. 2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/general.https-expected.txt.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Xabier Rodriguez-Calvar']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/1981498 Created attachment 287612 [details]
Patch
Comment on attachment 287612 [details] Patch Clearing flags on attachment: 287612 Committed r205289: <http://trac.webkit.org/changeset/205289> All reviewed patches have been landed. Closing bug. |