Bug 162556 – [Streams API] Align cancelReadableStream() with spec

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

[Romain Bellessort]

cancelReadableStream() behaviour is not fully in line with spec.

[Romain Bellessort]

Created attachment 289827 [details]
Patch

[Romain Bellessort]

Uploaded patches aligns the implementation of cancelReadableStream() with spec. Latest spec in particular defines a private method "cancel" in controller. This method is initialized in privateInitializeReadableStreamDefaultController (ReadableStreamInternals.js). This is similar to what was done for the private method "pull" introduced by latest spec.

The function name (cancelReadableStream) has also been modified to be aligned with spec (readableStreamCancel).

[Xabier Rodríguez Calvar]

Comment on attachment 289827 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:346
> -function cancelReadableStream(stream, reason)
> +function readableStreamCancel(stream, reason)

Are you planning to rename all methods? I mean not in this patch, but if you plan to rename these methods then I think we can go ahead. Otherwise I would stick to the current naming convention even when it differs from the spec one.

[youenn fablet]

Comment on attachment 289827 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:86
> +        "use strict";

We will create a different function for each controller.
Can we just create this function once and reuse it?
Something like this.@cancel = @readableStreamDefaultControllerCancel, readableStreamDefaultControllerCancel being defined in ReadableStreamInternals.js.

[Romain Bellessort]

(In reply to comment #3)
> Comment on attachment 289827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289827&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:346
> > -function cancelReadableStream(stream, reason)
> > +function readableStreamCancel(stream, reason)
> 
> Are you planning to rename all methods? I mean not in this patch, but if you
> plan to rename these methods then I think we can go ahead. Otherwise I would
> stick to the current naming convention even when it differs from the spec
> one.


Yes I am planning to do so. Prior to that, I want to align behaviour of some methods that is not yet fully aligned with spec (as in this patch).

[Romain Bellessort]

(In reply to comment #4)
> Comment on attachment 289827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289827&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:86
> > +        "use strict";
> 
> We will create a different function for each controller.
> Can we just create this function once and reuse it?
> Something like this.@cancel = @readableStreamDefaultControllerCancel,
> readableStreamDefaultControllerCancel being defined in
> ReadableStreamInternals.js.

Ok I'll do this (and I'll do the same for the similar pull() method).

[Romain Bellessort]

Created attachment 289938 [details]
Patch

[Romain Bellessort]

Comment on attachment 289938 [details]
Patch

The new patch implements the change suggested by Youenn (and applies it to the private "pull" method).

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:346
> > -function cancelReadableStream(stream, reason)
> > +function readableStreamCancel(stream, reason)
> 
> Are you planning to rename all methods? I mean not in this patch, but if you
> plan to rename these methods then I think we can go ahead. Otherwise I would
> stick to the current naming convention even when it differs from the spec
> one.

You didn't answer this :)

[Romain Bellessort]

(In reply to comment #9)
> (In reply to comment #3)
> > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:346
> > > -function cancelReadableStream(stream, reason)
> > > +function readableStreamCancel(stream, reason)
> > 
> > Are you planning to rename all methods? I mean not in this patch, but if you
> > plan to rename these methods then I think we can go ahead. Otherwise I would
> > stick to the current naming convention even when it differs from the spec
> > one.
> 
> You didn't answer this :)

I replied in comment #5 :) (yes, I am planing to rename all methods) (is it better to reply to multiple comments in a single comment in such cases?)

[Xabier Rodríguez Calvar]

(In reply to comment #10)
> I replied in comment #5 :) (yes, I am planing to rename all methods)

Oh, sorry, my bad, read too quickly.

> (is it
> better to reply to multiple comments in a single comment in such cases?)

You can do as you like.

[Xabier Rodríguez Calvar]

Comment on attachment 289938 [details]
Patch

Romain, I don't see you online so I won't set cq+ until you let me know on IRC.

[WebKit Commit Bot]

Comment on attachment 289938 [details]
Patch

Clearing flags on attachment: 289938

Committed r206508: <http://trac.webkit.org/changeset/206508>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.