Bug 141650

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.

Created attachment 246739 [details] Patch

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

Created attachment 247053 [details] Storage in ReadableStream

Created attachment 247054 [details] Storage as private in JSReadableStream

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

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

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

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

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.

Created attachment 247125 [details] Patch

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?

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.

Created attachment 247229 [details] Patch for landing

Comment on attachment 247229 [details] Patch for landing Clearing flags on attachment: 247229 Committed r180559: <http://trac.webkit.org/changeset/180559>

All reviewed patches have been landed. Closing bug.