Bug 143363

Created attachment 250060 [details] Patch

Created attachment 250434 [details] Patch

Comment on attachment 250434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250434&action=review Very quick review. Something is fishy. If that's right, please add tests for the controller. Can you please add the type exception on errored? It is not testable yet, but it may be harder to forget if there is already if branch for it. > Source/WebCore/Modules/streams/ReadableStream.cpp:61 > +void ReadableStream::changeStateToClosed() How will you raise the TypeError when closeRequested == true or state == "errored"? > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127 > + getReadableJSStream(exec).changeStateToClosed(); > + return JSValue::encode(jsUndefined()); Hum, reading the spec side by side with the code...I don't think that is quite correct. Say I keep the function close in a variable close. I could do: close.call("foobar"); and that would still close the original stream. The specs says to get the internal slot on the this object. If the this object is not a controller: If IsReadableStreamController(this) is false, throw a TypeError exception. Note that this also let you do close.call(otherController) to close the controller of one object with the close function from an other object. If I am reading this correctly, you need to have the internal slot on the controller object, and get it out through the this object.

(In reply to comment #3) > Comment on attachment 250434 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250434&action=review > > Very quick review. Something is fishy. If that's right, please add tests for > the controller. > > Can you please add the type exception on errored? It is not testable yet, > but it may be harder to forget if there is already if branch for it. > > > Source/WebCore/Modules/streams/ReadableStream.cpp:61 > > +void ReadableStream::changeStateToClosed() > > How will you raise the TypeError when closeRequested == true or state == > "errored"? > > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127 > > + getReadableJSStream(exec).changeStateToClosed(); > > + return JSValue::encode(jsUndefined()); > > Hum, reading the spec side by side with the code...I don't think that is > quite correct. > > Say I keep the function close in a variable close. I could do: > close.call("foobar"); > and that would still close the original stream. > > The specs says to get the internal slot on the this object. If the this > object is not a controller: > If IsReadableStreamController(this) is false, throw a TypeError > exception. > > Note that this also let you do close.call(otherController) to close the > controller of one object with the close function from an other object. > > If I am reading this correctly, you need to have the internal slot on the > controller object, and get it out through the this object. You are reading correctly. I was expecting to first do that patch and then upgrade the controller implementation so that it matches the spec and ensure close is still working. But I can do it the other way as well. For the controller, I am not yet sure whether going the IDL way or making a full custom implementation of it.

(In reply to comment #4) > > If I am reading this correctly, you need to have the internal slot on the > > controller object, and get it out through the this object. > > You are reading correctly. > I was expecting to first do that patch and then upgrade the controller > implementation so that it matches the spec and ensure close is still working. > But I can do it the other way as well. > > For the controller, I am not yet sure whether going the IDL way or making a > full custom implementation of it. A fix in a follow up is okay with me. I r+ed this one. Poke me when you have the follow up.

Created attachment 250702 [details] Patch for landing

Comment on attachment 250702 [details] Patch for landing Clearing flags on attachment: 250702 Committed r182794: <http://trac.webkit.org/changeset/182794>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 143714

Created attachment 251421 [details] Patch Rebased against trunk.

Comment on attachment 251421 [details] Patch Clearing flags on attachment: 251421 Committed r183395: <http://trac.webkit.org/changeset/183395>

All reviewed patches have been landed. Closing bug.