Bug 160299 – [Streams API] Align internal structure of ReadableStream with spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Romain Bellessort]

ReadableStream internal structure (and corresponding behaviors) have been modified with the introduction of ReadableStreamDefaultController and ReadableByteStreamController. Aligning with spec should be done prior to considering adding ReadableByteStreamController.

[Romain Bellessort]

Created attachment 284789 [details]
Patch

[Romain Bellessort]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Romain Bellessort]

Created attachment 285116 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Romain Bellessort]

Created attachment 285211 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Romain Bellessort]

Created attachment 285216 [details]
Patch

[Romain Bellessort]

(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.

[Xabier Rodríguez Calvar]

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.

[Romain Bellessort]

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.

[Xabier Rodríguez Calvar]

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 ([e-mail]) {
> -        const chunk = @dequeueValue(stream.@queue);
> -        if (stream.@closeRequested && [e-mail] === 0)
> -            @finishClosingReadableStream(stream);
> +    if (stream.@[e-mail]) {
> +        const chunk = @dequeueValue(stream.@readableStreamController.@queue);
> +        if (stream.@readableStreamController.@closeRequested && stream.@[e-mail] === 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.

[Xabier Rodríguez Calvar]

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";

[youenn fablet]

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.

[Romain Bellessort]

Created attachment 285330 [details]
Patch

[Romain Bellessort]

(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 ([e-mail]) {
> > -        const chunk = @dequeueValue(stream.@queue);
> > -        if (stream.@closeRequested && [e-mail] === 0)
> > -            @finishClosingReadableStream(stream);
> > +    if (stream.@[e-mail]) {
> > +        const chunk = @dequeueValue(stream.@readableStreamController.@queue);
> > +        if (stream.@readableStreamController.@closeRequested && stream.@[e-mail] === 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.

[Xabier Rodríguez Calvar]

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.

[Romain Bellessort]

Created attachment 285336 [details]
Patch

[Romain Bellessort]

(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.

[youenn fablet]

> > > 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.

[youenn fablet]

In this bug, it might be best to keep it like it is now and address this in another bug.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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.

[Romain Bellessort]

Created attachment 287511 [details]
Patch

[WebKit Commit Bot]

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

[Romain Bellessort]

Created attachment 287612 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287612 [details]
Patch

Clearing flags on attachment: 287612

Committed r205289: <http://trac.webkit.org/changeset/205289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.