[Streams API] Reading ReadableStream ready and closed attributes should not always create a new promise

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

As discussed in https://bugs.webkit.org/attachment.cgi?id=246100&action=review, the current implementation of ReadableStream ready attribute leads to the creation of a new promise object each time one the ready attribute is read.
This should be fixed so that the same promise object is returned.
This also applies to the closed attribute.

[youenn fablet]

The simplest solution is probably to store a DeferredWrapper either at ReadableStream or JSReadableStream side.
Given that we try to keep ReadableStream away from DeferredWrapper, the idea would be to modify the binding generator to manage DeferredWrapper at JSXXX classes, whenever a Promise-type attribute happens in the corresponding IDL.

[youenn fablet]

Created attachment 246739 [details]
Patch

[youenn fablet]

(In reply to comment #2)
> Created attachment 246739 [details]
> Patch

Patch stores promise as JSReadableStream slots and adds a new DeferredWrapper constructor to use that stored promise.

[youenn fablet]

Created attachment 247053 [details]
Storage in ReadableStream

[youenn fablet]

Created attachment 247054 [details]
Storage as private in JSReadableStream

[youenn fablet]

Created attachment 247084 [details]
Storage as private in JSReadableStream - trying to fix mac build

[youenn fablet]

Created attachment 247085 [details]
Storage as private in JSReadableStream - trying to fix mac build again

[youenn fablet]

Created attachment 247107 [details]
Storage as private in JSReadableStream - fixing mac build

[youenn fablet]

(In reply to comment #8)
> Created attachment 247107 [details]
> Storage as private in JSReadableStream - fixing mac build

This patch stores readyPromise and closedPromise as internal slots of JSReadableStream. This is similar to what is described in https://streams.spec.whatwg.org/#rs-internal-slots.
Internal slots are also used in the last patch of bug 141160 to store a reference of the JSReadableStream object directly in the enqueue, close and error JS functions passed as parameters to the start and pull callbacks.

An alternative to internal slots is to store DeferredWrapper objects in ReadableStream (corresponding patch is https://bugs.webkit.org/attachment.cgi?id=247053&action=review).
I have a preference to the first approach.

In a longer term, if this pattern happens in other APIs, it may be best to make the promise objects members of the JSReadableStream class.

[Benjamin Poulain]

Comment on attachment 247107 [details]
Storage as private in JSReadableStream - fixing mac build

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

> LayoutTests/streams/readablestream-constructor.html:69
> +    assert_array_equals(Object.keys(rs), ['closed', 'ready']);

It would be interesting to test Object.getOwnPropertyNames() as well. Object.keys() only gives you enumerable properties.

[youenn fablet]

Created attachment 247125 [details]
Patch

[Benjamin Poulain]

Comment on attachment 247125 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Stored ready and closed JSPromiseDeferred as slots in JSReadableStream.
> +        Added new constructor for DeferredWrapper to use these stored values.
> +        Updated ready and closed methods to use that new constructor.
> +        Changes are covered by new test in streams/readablestream-constructor.html.

I am not a big fan of private names but I won't fight you on that. Your tests verify the correctness of this, we can change the implementation later if needed.

What I would like is a complete description in the change log. Explain the problem, two possible solutions, and give the rationale for picking private names above internal states on ReadableStream. This kind of information will be useful to anyone who wants to refactor this code in the future.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:60
> +        return jsUndefined();

I think an invalid JSValue() would be semantically better here. A private slot could contained undefined.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:66
> +void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);
> +JSC::JSValue getInternalSlotFromObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&);

Isn't that a weird place to put this?

Why not just make them static in JSReadableStreamCustom.cpp?

[youenn fablet]

Thanks for the review.

(In reply to comment #12)
> Comment on attachment 247125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247125&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Stored ready and closed JSPromiseDeferred as slots in JSReadableStream.
> > +        Added new constructor for DeferredWrapper to use these stored values.
> > +        Updated ready and closed methods to use that new constructor.
> > +        Changes are covered by new test in streams/readablestream-constructor.html.
> 
> I am not a big fan of private names but I won't fight you on that. Your
> tests verify the correctness of this, we can change the implementation later
> if needed.
> 

The current patch centralizes most things to refactor in JSReadableStreamCustom, making future refactoring a bit easier.

> What I would like is a complete description in the change log. Explain the
> problem, two possible solutions, and give the rationale for picking private
> names above internal states on ReadableStream. This kind of information will
> be useful to anyone who wants to refactor this code in the future.

Sounds good

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:60
> > +        return jsUndefined();
> 
> I think an invalid JSValue() would be semantically better here. A private
> slot could contained undefined.

OK

> 
> > Source/WebCore/bindings/js/ReadableStreamJSSource.h:66
> > +void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);
> > +JSC::JSValue getInternalSlotFromObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&);
> 
> Isn't that a weird place to put this?
> 
> Why not just make them static in JSReadableStreamCustom.cpp?

These functions were first developped for ReadableStreamJSSource and will be used for subsequent patches (https://bugs.webkit.org/attachment.cgi?id=247116&action=review for example).

Ideally these helper functions should go in some other places as they are not tied to ReadableStream. But I did not found any obvious place for them.

[youenn fablet]

Created attachment 247229 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 247229 [details]
Patch for landing

Clearing flags on attachment: 247229

Committed r180559: <http://trac.webkit.org/changeset/180559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.