cancelReadableStream() behaviour is not fully in line with spec.
Created attachment 289827 [details] Patch
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).
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.
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.
(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).
(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).
Created attachment 289938 [details] Patch
Comment on attachment 289938 [details] Patch The new patch implements the change suggested by Youenn (and applies it to the private "pull" method).
(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 :)
(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?)
(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.
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.
Comment on attachment 289938 [details] Patch Clearing flags on attachment: 289938 Committed r206508: <http://trac.webkit.org/changeset/206508>
All reviewed patches have been landed. Closing bug.