Bug 141045

Created attachment 245644 [details] Patch

Created attachment 245645 [details] Patch

Created attachment 245649 [details] Patch

Created attachment 245875 [details] Rebasing and minor style improvements

Comment on attachment 245875 [details] Rebasing and minor style improvements View in context: https://bugs.webkit.org/attachment.cgi?id=245875&action=review Sorry, I only had time for a super quick review. It is great you split the patch is smaller chunk, it is much easier to verify. My main concerns so far are: -It is weird JSReadableStream requires so much custom bindings. Custom Bindings are supposed to be an exception for performance related stuff. It may be better to extend the binding generator. -Unless I am missing JSReadableStreamSource will be for, the name is incorrect. The JSPrefix is generally for objects on the JS heap, which is not the case here. > Source/WebCore/Modules/streams/ReadableStream.cpp:84 > + // FIXME: To be implemented You can use notImplemented() to get logging of missing stuff. > Source/WebCore/Modules/streams/ReadableStream.cpp:89 > + // FIXME: To be implemented diitto > Source/WebCore/Modules/streams/ReadableStream.idl:36 > + readonly attribute ReadableStreamStateType state; Two spaces between readonly and attribute. Shouldn't this be [GetterRaisesException]? The spec seems to say this can throw if the readable stream has no source: https://streams.spec.whatwg.org/#rs-state > Source/WebCore/Modules/streams/ReadableStream.idl:44 > + [GetterRaisesException, CustomGetter] readonly attribute Promise closed; > + [GetterRaisesException, CustomGetter] readonly attribute Promise ready; On the other hand I don't get why those two can raise. Shouldn't they just return a rejected promise? > Source/WebCore/Modules/streams/ReadableStreamSource.h:49 > + UNUSED_PARAM(queueSize); We just omit the param in C++. UNUSED_PARAM is mostly for Obj-C. > Source/WebCore/Modules/streams/ReadableStreamSource.h:56 > + UNUSED_PARAM(reason); ditto. > ChangeLog:9 > + * Source/cmake/WebKitFeatures.cmake: > + * Source/cmakeconfig.h.cmake: Made streams API compilation on by default. It would be useful to Object.getOwnPropertyDescriptor() the whole interface. It's good to check we expose the attributes we expect.

(In reply to comment #5) > Comment on attachment 245875 [details] > Rebasing and minor style improvements > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245875&action=review > > Sorry, I only had time for a super quick review. Thanks for taking that time, this is much appreciated! > > It is great you split the patch is smaller chunk, it is much easier to > verify. > > My main concerns so far are: > -It is weird JSReadableStream requires so much custom bindings. Custom > Bindings are supposed to be an exception for performance related stuff. It > may be better to extend the binding generator. Currently use of promise requires going through custom bindings, which ReadableStream is all about, except for state and read(). The current patch tries as much as possible to have a JS-free ReadableStream definition. It would be great to ease Promise usage, but it sounds a bit premature to me right now. Looking at Crypto promise usage, it seems not straightforward to pick the right model that would suit most Promise-based APIs (error handling, resolve type at IDL level...). I would be interested in looking further if/when there are other promise-based APIs on the go. I guess the crux of the issue is that streams are really in between JS and WebCore. > -Unless I am missing JSReadableStreamSource will be for, the name is > incorrect. The JSPrefix is generally for objects on the JS heap, which is > not the case here. OK, let's rename it to ReadabbleStreamJSSource then. > > > Source/WebCore/Modules/streams/ReadableStream.cpp:84 > > + // FIXME: To be implemented > > You can use notImplemented() to get logging of missing stuff. OK > > > Source/WebCore/Modules/streams/ReadableStream.cpp:89 > > + // FIXME: To be implemented > > diitto > > > Source/WebCore/Modules/streams/ReadableStream.idl:36 > > + readonly attribute ReadableStreamStateType state; > > Two spaces between readonly and attribute. > > Shouldn't this be [GetterRaisesException]? The spec seems to say this can > throw if the readable stream has no source: > https://streams.spec.whatwg.org/#rs-state A ReadableStream always has a source with the current design. We can fix that later if there is a use case for a ReadableStream without a source. > > Source/WebCore/Modules/streams/ReadableStream.idl:44 > > + [GetterRaisesException, CustomGetter] readonly attribute Promise closed; > > + [GetterRaisesException, CustomGetter] readonly attribute Promise ready; > > On the other hand I don't get why those two can raise. Shouldn't they just > return a rejected promise? Historical reasons... I will fix that. > > > Source/WebCore/Modules/streams/ReadableStreamSource.h:49 > > + UNUSED_PARAM(queueSize); > > We just omit the param in C++. OK > > UNUSED_PARAM is mostly for Obj-C. > > > Source/WebCore/Modules/streams/ReadableStreamSource.h:56 > > + UNUSED_PARAM(reason); > > ditto. > > > ChangeLog:9 > > + * Source/cmake/WebKitFeatures.cmake: > > + * Source/cmakeconfig.h.cmake: Made streams API compilation on by default. > > It would be useful to Object.getOwnPropertyDescriptor() the whole interface. > It's good to check we expose the attributes we expect. I am not sure to get it. LayoutTests/js/dom/global-constructors-attributes.html is capturing ReadableStream properties. LayoutTests/streams/readablestream-constructor.html is checking each expected property and its type or value. I will beef up a little bit this test to ensure that no unexpected attribute is exposed.

Created attachment 245939 [details] Patch

Adding the bindings generator expert.

> > > ChangeLog:9 > > > + * Source/cmake/WebKitFeatures.cmake: > > > + * Source/cmakeconfig.h.cmake: Made streams API compilation on by default. > > > > It would be useful to Object.getOwnPropertyDescriptor() the whole interface. > > It's good to check we expose the attributes we expect. > > I am not sure to get it. > LayoutTests/js/dom/global-constructors-attributes.html is capturing > ReadableStream properties. > LayoutTests/streams/readablestream-constructor.html is checking each > expected property and its type or value. > I will beef up a little bit this test to ensure that no unexpected attribute > is exposed. Maybe I am the one missing the test. You are adding 3 enumerable properties and a few calls, but I don't see their properties dumped anywhere (see LayoutTests/fast/dom/SelectorAPI/closest-definition.html for a recent example of missing test coverage).

Comment on attachment 245939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245939&action=review This patch is great. Let's move forward. Please have a look at the extra tests for your Interface before landing. The more coverage we have the better. > Source/WebCore/Modules/streams/ReadableStream.cpp:33 > +#include "ReadableStream.h" Since ReadableStream.h includes the ENABLE(STREAMS_API) guards, you can use the regular style for includes: #include "config.h" #include "ReadableStream.h" .... > Source/WebCore/Modules/streams/ReadableStream.cpp:56 > + readableStreamCounter.increment(); You may want to add a dump() function with #ifndef NDEBUG to show the counter. I have found such little helper to be useful for me when debugging. > Source/WebCore/Modules/streams/ReadableStream.idl:30 > +enum ReadableStreamStateType { "readable", "waiting", "closed", "errored" }; Two spaces between "readable" and "waiting"

Created attachment 246094 [details] Patch for landing

Comment on attachment 246094 [details] Patch for landing Clearing flags on attachment: 246094 Committed r179687: <http://trac.webkit.org/changeset/179687>

All reviewed patches have been landed. Closing bug.