Bug 147092 – Migrate streams API to JS Builtins

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Streams API is currently being implemented  in WebCore.
It might be worth investigating moving it to JSC.

[Xabier Rodríguez Calvar]

Some history: The implementation of Streams API began in WebCore. It is true that the border could be fuzzy but given that it is a Web spec, not JS and that we need to add much more code to implement it in JS than in WebCore, I think the proper place would be WebCore.

You can have bug 146594 as a reference.

[youenn fablet]

Some interesting point of discussions:

Sam mentioned that streams should be implemented similarly to promise.
Implementing that with the help of JSC builtins would be probably nice indeed.

Geoff mentioned that the streams API should not be visible from JS environments but only from DOM environments.
Reason is that there is no plan to have streams in ES6 or ES7.
That would lead me to think we should stick for a moment with WebCore.

Using JSC builtins is nevertheless tempting for some features like ReadableStream::tee, maybe pipeTo as well.

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> Geoff mentioned that the streams API should not be visible from JS
> environments but only from DOM environments.
> Reason is that there is no plan to have streams in ES6 or ES7.
> That would lead me to think we should stick for a moment with WebCore.

Agree.

[Sam Weinig]

(In reply to comment #3)
> (In reply to comment #2)
> > Geoff mentioned that the streams API should not be visible from JS
> > environments but only from DOM environments.
> > Reason is that there is no plan to have streams in ES6 or ES7.
> > That would lead me to think we should stick for a moment with WebCore.
> 
> Agree.

I don't 100% agree, but I also see no reason to block you. If we decide later it should have been in JSC, we can move it.

[youenn fablet]

> I don't 100% agree, but I also see no reason to block you. If we decide
> later it should have been in JSC, we can move it.

Can you elaborate?
Would you think we could implement it in JSC but only surface public APIs in WebCore through JSDOMGlobalObject?

[youenn fablet]

Created attachment 258476 [details]
Gauging migration to JSC

[youenn fablet]

Created attachment 258483 [details]
Gauging migration to JSC (+some comments)

[Xabier Rodríguez Calvar]

Comment on attachment 258483 [details]
Gauging migration to JSC (+some comments)

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

I didn't do a thorough review of the JS code and if it is compliant with the spec, which I suspect it is because tests seem to be passing silky smooth. Just commented a couple of things of style and stuff.
I think the code should live in WebCore and not only the code, but the instantiated objects as well as I'll explain later.
Other concern that I have is efficiency, cause my lack of knowledge doesn't allow me to guess when the JS builtin code is compiled and how it affects performance.
About the buffers and chunks movement, I guess we can figure out something as we can link with native C++ code.
Other than that, the solution looks good overall because it can help us to get closer to the spec and it would be more maintainable.

> Source/JavaScriptCore/CMakeLists.txt:534
> +    runtime/JSReadableStream.cpp
> +    runtime/JSReadableStreamController.cpp
> +    runtime/JSReadableStreamReader.cpp

If it is going to be enabled only in DOM environments, code should live in WebCore.

> Source/JavaScriptCore/CMakeLists.txt:653
> +    runtime/JSReadableStream.cpp
> +    runtime/JSReadableStreamController.cpp
> +    runtime/JSReadableStreamReader.cpp

It is true that moving it to WebCore can make some things more difficult and maybe the lut files generation can be one of them.

> Source/JavaScriptCore/builtins/ReadableStream.js:80
> +        throw new TypeError();

Provide error string.

> Source/JavaScriptCore/builtins/ReadableStream.js:82
> +        throw new RangeError();

Ditto.

> Source/JavaScriptCore/builtins/ReadableStream.js:102
> +       throw new @TypeError("");

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.js:104
> +       throw new @TypeError("");

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.js:137
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.js:139
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.js:309
> +    }
> +    catch(error) {

Same line?

> Source/JavaScriptCore/builtins/ReadableStream.js:367
> +    }
> +    catch(error) {

Ditto.

> Source/JavaScriptCore/builtins/ReadableStream.prototype.js:46
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.prototype.js:49
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStream.prototype.js:68
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:33
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:40
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:50
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:54
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:64
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:68
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:71
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamController.prototype.js:81
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamReader.prototype.js:33
> +        return Promise.reject(new @TypeError());

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamReader.prototype.js:51
> +        return Promise.reject(new @TypeError());

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamReader.prototype.js:61
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/builtins/ReadableStreamReader.prototype.js:67
> +        throw new @TypeError();

Ditto

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:156
> +#if ENABLE(STREAMS_API)
> +#include "JSReadableStream.h"
> +#include "JSReadableStreamController.h"
> +#include "JSReadableStreamReader.h"
> +#endif

So from what I see you are adding all these objects and initializing them already in the runtime and the only thing that is going to be done later is adding the constructor to the DOMWindow. Am I right. If we do so we are bloating the memory in the runtime with something that is not going to be available in the end. That's why I think all these objects and specially their initializations should be moved to WebCore. I don't know if DOMWindow or JSDOMGlobalObject is the right place and even which is the best way of putting it there, but it should be definitely out of here, IMHO. Btw, I quote these includes because it is too much to quote the whole file :)

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:453
> -    
> -    
> +

I wouldn't touch this part even if it removes a trailing space because it does not have to do with the patch. In the other parts of the code it is ok, but I'd leave this out of the way because it "pollutes" the diff.

> Source/JavaScriptCore/runtime/JSReadableStream.cpp:38
> +namespace JSC {

Of course we would change the namespace if we lived in WebCore

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:22743
> -</Project>
>  \ No newline at end of file
> +</Project>

Careful here, no need to touch this.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:15418
> -</Project>
>  \ No newline at end of file
> +</Project>

Ditto.

> LayoutTests/js/dom/global-constructors-attributes-expected.txt:-945
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').value is ReadableStream
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').hasOwnProperty('get') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').hasOwnProperty('set') is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').enumerable is false
> -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').configurable is true

I wonder if all these things should change or not. I don't think they should, should they?

> LayoutTests/streams/reference-implementation/brand-checks-expected.txt:4
> -FAIL ReadableStream.prototype.pipeThrough works generically on its this and its arguments Can only call ReadableStream.pipeThrough on instances of ReadableStream
> +PASS ReadableStream.prototype.pipeThrough works generically on its this and its arguments 

Nice to see these passing.

[youenn fablet]

Thanks for the feedback.

This patch is a first try at moving streams to JSC.
Overall, I like the way it simplifies the architecture and code, it is just so much easier to implement...
I felt like there was some limited slowdown when running the tests but i did no actual measurement.

It also opens some potential better handling of native sources.
Next step is precisely to evaluate the link with WebCore native sources.
I think this should give us enough data to make an informed choice.

> Other concern that I have is efficiency, cause my lack of knowledge doesn't
> allow me to guess when the JS builtin code is compiled and how it affects
> performance.

That is a fair concern. 
I may be wrong but I think it is compiled when the builtin function is called.
This seems ok to me.

The other efficiency concern might be memory copies between consumer and producer.
Dedicated piping might be used in the places we want to optimize things like HTTP -> MediaStream.

> If it is going to be enabled only in DOM environments, code should live in
> WebCore.

I tend to agree, but we must weight pros and cons.
Can you be more specific?

FWIW, a V8 implementation is/was under consideration (see https://code.google.com/p/chromium/issues/detail?id=240603#c36).
Streams might be useful for JS-only environments at some point.

> > Source/JavaScriptCore/builtins/ReadableStream.js:80
> > +        throw new TypeError();
> 
> Provide error string.

There is a TODO in the js file.
Plan to do that if/when finalizing this patch.

> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:156
> > +#if ENABLE(STREAMS_API)
> > +#include "JSReadableStream.h"
> > +#include "JSReadableStreamController.h"
> > +#include "JSReadableStreamReader.h"
> > +#endif
> 
> So from what I see you are adding all these objects and initializing them
> already in the runtime and the only thing that is going to be done later is
> adding the constructor to the DOMWindow. Am I right.

Yes.

> If we do so we are
> bloating the memory in the runtime with something that is not going to be
> available in the end. That's why I think all these objects and specially
> their initializations should be moved to WebCore. I don't know if DOMWindow
> or JSDOMGlobalObject is the right place and even which is the best way of
> putting it there, but it should be definitely out of here, IMHO. Btw, I
> quote these includes because it is too much to quote the whole file :)

When JSC is used on its own, it might bloat things a bit, especially if we remove the compilation flag.
I do not think we are bloating the memory in WebKit case.

We could try to leave as much code as possible in WebCore, but I do not think we could directly have it all there.
It might be just better to have it all in JSC, except some small bits, native source and (temporarily?) constructor insertion.

Anyway, this is a good discussion to have.
Any additional feedback welcome.

> > LayoutTests/js/dom/global-constructors-attributes-expected.txt:-945
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').value is ReadableStream
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').hasOwnProperty('get') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').hasOwnProperty('set') is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').enumerable is false
> > -PASS Object.getOwnPropertyDescriptor(global, 'ReadableStream').configurable is true
> 
> I wonder if all these things should change or not. I don't think they
> should, should they?

I did not have yet time to study this precisely.
Plan to do when/if finalizing the patch.

[Xabier Rodríguez Calvar]

(In reply to comment #9)
> 
> > If it is going to be enabled only in DOM environments, code should live in
> > WebCore.
> 
> I tend to agree, but we must weight pros and cons.
> Can you be more specific?
> 
> FWIW, a V8 implementation is/was under consideration (see
> https://code.google.com/p/chromium/issues/detail?id=240603#c36).

IMHO, if it is enable only for the DOM environments the code should live in WebCore and have that namespace instead of JSC.

> Streams might be useful for JS-only environments at some point.

It could be, but it is a too long shot, IMHO.

> When JSC is used on its own, it might bloat things a bit, especially if we
> remove the compilation flag.
> I do not think we are bloating the memory in WebKit case.

We wouldn't be bloating the memory for the DOM case, of course, we would be adding unnecessary objects for JS only environments if we don't force the compilation to remove it.

> We could try to leave as much code as possible in WebCore, but I do not
> think we could directly have it all there.
> It might be just better to have it all in JSC, except some small bits,
> native source and (temporarily?) constructor insertion.

If we have it in WebCore, this part of the code has to go to WebCore too so we won't have having this issue anymore.

About the rest, I'm ok.

[youenn fablet]

Created attachment 259597 [details]
Patch

[WebKit Commit Bot]

Attachment 259597 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:80:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:668:  Alphabetical sorting problem. "runtime/JSReadableStream.cpp" should be before "runtime/ModuleLoaderObject.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 10 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> Created attachment 259597 [details]
> Patch

You didn't fix some of the things I suggested, it makes no sense to me to discuss them again, it was just my opinion :)

[youenn fablet]

Created attachment 259611 [details]
Trying to fix mac build

[WebKit Commit Bot]

Attachment 259611 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:52:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStream.h:80:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamController.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:668:  Alphabetical sorting problem. "runtime/JSReadableStream.cpp" should be before "runtime/ModuleLoaderObject.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSReadableStreamReader.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 10 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

I did a quick mock-up version of native sources (see bug 138968) based on JSC ReadableStream. In most respects, moving to JSC builtins and JSXX structures are real improvements to me.

The JSXX classes could be located in WebCore.
I put them in JSC since builtins are currently only available in JSC.
If we envision adding WebCore JS builtins at some point, we could try keeping ReadableStream in WebCore. But, even for pure JS environments, streams may be useful in the future. The same applies to WritableStream of course.

If that is not too much effort, I would try to clean WebCore ReadableStream go with pending patches (bug 148075 and bug 146278) before removing it.

[youenn fablet]

Comment on attachment 259611 [details]
Trying to fix mac build

I know the patch is a bit big but it allows keeping the same level of functionality.
Let me know if that would ease things to split it.

[Darin Adler]

Comment on attachment 259611 [details]
Trying to fix mac build

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

I’m not qualified to review the whole thing, but looks good to me.

> Source/JavaScriptCore/builtins/ReadableStream.js:59
> +    if (strategy !== undefined && !@isObject(strategy))

How can strategy === undefined given the typeof strategy === "undefined" check above?

> Source/JavaScriptCore/builtins/ReadableStream.js:72
> +    this.@reader = undefined;
> +    this.@storedError = undefined;

Is this better than just not setting the property? Is it undefined we want, rather than, say, null? I suppose setting the property is better for performance than leaving it not set.

> Source/JavaScriptCore/builtins/ReadableStream.js:75
> +    this.@highWaterMark = Number(strategy.highWaterMark);

Is this really the right thing to do for the number? Does this unnecessarily box the number? Maybe we should be using "+ 0" or some other idiom instead.

> Source/JavaScriptCore/builtins/ReadableStream.js:218
> +    reader.@ownerReadableStream.@reader = undefined;

Can this function be called when @ownerReadableStream is already null?

Do we have test coverage for the fact that this sets reader to undefined specifically rather than, say, null?

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:334
> +    macro(state) \
> +    macro(underlyingSource) \
> +    macro(queue) \
> +    macro(queueSize) \
> +    macro(started) \
> +    macro(closeRequested) \
> +    macro(pullAgain) \
> +    macro(pulling) \
> +    macro(reader) \
> +    macro(storedError) \
> +    macro(controller) \
> +    macro(strategySize) \
> +    macro(highWaterMark) \
> +    macro(readRequests) \
> +    macro(ownerReadableStream) \
> +    macro(closedPromise) \
> +    macro(closedPromiseResolve) \
> +    macro(closedPromiseReject) \
> +    macro(controlledReadableStream) \
> +    macro(readableStreamClosed) \
> +    macro(readableStreamReadable) \
> +    macro(readableStreamErrored) \
> +    macro(ReadableStreamReader) \
> +    macro(ReadableStreamController) \

I think we should be keeping this list sorted instead of in order things were added. I know not everything was already done that way.

[youenn fablet]

Comment on attachment 259611 [details]
Trying to fix mac build

Thanks for the review.

Your feedback might lead to some changes in the patch.
I think these changes can be done at cq time or as a next patch if more changes are required.

Please see inline for additional comments/answers.

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

>> Source/JavaScriptCore/builtins/ReadableStream.js:59
>> +    if (strategy !== undefined && !@isObject(strategy))
> 
> How can strategy === undefined given the typeof strategy === "undefined" check above?

Right, I will change this in a next patch or at cq time.

>> Source/JavaScriptCore/builtins/ReadableStream.js:72
>> +    this.@storedError = undefined;
> 
> Is this better than just not setting the property? Is it undefined we want, rather than, say, null? I suppose setting the property is better for performance than leaving it not set.

This is mostly to follow step-by-step the spec.
Although this might not be necessary now, I would prefer keeping it at the moment.

>> Source/JavaScriptCore/builtins/ReadableStream.js:75
>> +    this.@highWaterMark = Number(strategy.highWaterMark);
> 
> Is this really the right thing to do for the number? Does this unnecessarily box the number? Maybe we should be using "+ 0" or some other idiom instead.

The spec mandates it.
We would probably loose the case of HWM set to "Infinity".

>> Source/JavaScriptCore/builtins/ReadableStream.js:218
>> +    reader.@ownerReadableStream.@reader = undefined;
> 
> Can this function be called when @ownerReadableStream is already null?
> 
> Do we have test coverage for the fact that this sets reader to undefined specifically rather than, say, null?

This is fully covered in the test suite (close, error and releasing).

The behaviour in the spec has changed a bit here since I authored these lines.
This implementation is synced with https://streams.spec.whatwg.org/commit-snapshots/0e56ed49dc66fd1b29cd08acaafad24a7c06f058/
Plan would be to upstream support for that particular revision, then update to the latest snapshot.

I will mention https://streams.spec.whatwg.org/commit-snapshots/0e56ed49dc66fd1b29cd08acaafad24a7c06f058/ as a comment within the Readabestream*.js files in a next patch or at cq time.

>> Source/JavaScriptCore/runtime/CommonIdentifiers.h:334
>> +    macro(ReadableStreamController) \
> 
> I think we should be keeping this list sorted instead of in order things were added. I know not everything was already done that way.

OK, let's do that as a follow-up patch then.

[Xabier Rodríguez Calvar]

Comment on attachment 259611 [details]
Trying to fix mac build

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

> Source/JavaScriptCore/builtins/ReadableStream.js:332
> +function invokeOrNoop(object, key, args)

About this I wonder two things:
Should we prefix or postfix these more general functions to avoid any collision in naming? In these cases I don't think they can collide, but in my implementation of WritableStreams I created some functions to resolve and reject promises more easily and they collide with the Promises ones.
Should we move these functions (all or only the general ones) to a Streams.js file that could be used by both implementations?

[Xabier Rodríguez Calvar]

Comment on attachment 259611 [details]
Trying to fix mac build

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

> Source/JavaScriptCore/builtins/ReadableStream.js:65
> +    this.@queue = [];
> +    this.@queueSize = 0;

For Writable I created an object here with "content" being the queue and size being the obvious. That way it is easier to create the functions the spec requires with the same parameters and still keep some general implementation that could be used for both readable and writable (peek, dequeue, etc)

> Source/JavaScriptCore/builtins/ReadableStream.js:113
> +        this.@closedPromise = new Promise(function(resolve, reject) {
> +            stream.@reader.@closedPromiseResolve = resolve;
> +            stream.@reader.@closedPromiseReject = reject;
> +        });

As I said before, I created three functions newStreamsPromise, revolveStreamsPromise and rejectStreams promise that use only one slot and keep the resolve and reject functions as attributes inside the promise object. Then the resolve and reject functions check that the promise is not undefined and the corresponding method exist, invoke then and unset them.

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 259611 [details]
> Trying to fix mac build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259611&action=review
> 
> > Source/JavaScriptCore/builtins/ReadableStream.js:65
> > +    this.@queue = [];
> > +    this.@queueSize = 0;
> 
> For Writable I created an object here with "content" being the queue and
> size being the obvious. That way it is easier to create the functions the
> spec requires with the same parameters and still keep some general
> implementation that could be used for both readable and writable (peek,
> dequeue, etc)
> 
> > Source/JavaScriptCore/builtins/ReadableStream.js:113
> > +        this.@closedPromise = new Promise(function(resolve, reject) {
> > +            stream.@reader.@closedPromiseResolve = resolve;
> > +            stream.@reader.@closedPromiseReject = reject;
> > +        });
> 
> As I said before, I created three functions newStreamsPromise,
> revolveStreamsPromise and rejectStreams promise that use only one slot and
> keep the resolve and reject functions as attributes inside the promise
> object. Then the resolve and reject functions check that the promise is not
> undefined and the corresponding method exist, invoke then and unset them.

This refactoring might be interesting.

I would tend to introduce the refactored routines at the time we saw a potential redundancy in the upstreamed code. That would allow us easy evaluation of the refactoring benefits and the best way to achieve it.

[youenn fablet]

(In reply to comment #20)
> Comment on attachment 259611 [details]
> Trying to fix mac build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259611&action=review
> 
> > Source/JavaScriptCore/builtins/ReadableStream.js:332
> > +function invokeOrNoop(object, key, args)
> 
> About this I wonder two things:
> Should we prefix or postfix these more general functions to avoid any
> collision in naming? In these cases I don't think they can collide, but in
> my implementation of WritableStreams I created some functions to resolve and
> reject promises more easily and they collide with the Promises ones.
> Should we move these functions (all or only the general ones) to a
> Streams.js file that could be used by both implementations?

I am not quite sure. It would be good to have opinion from others.
Here is my take on it.
Prefixing/Postfixing such routines seems good to me.
invokeOrNoop is defined in the streams API spec. Though it is generic, I would leave it into streams scope.
For the promise routines, streams scope might make less sense.

[Darin Adler]

Comment on attachment 259611 [details]
Trying to fix mac build

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

>>> Source/JavaScriptCore/builtins/ReadableStream.js:75
>>> +    this.@highWaterMark = Number(strategy.highWaterMark);
>> 
>> Is this really the right thing to do for the number? Does this unnecessarily box the number? Maybe we should be using "+ 0" or some other idiom instead.
> 
> The spec mandates it.
> We would probably loose the case of HWM set to "Infinity".

No, we wouldn’t lose infinity. You can add zero to infinity and you will get infinity. To get this right it would be better to understand clearly what the spec mandates.

[Darin Adler]

Who’s going to be the primary reviewer for this. Geoff, can it be you? Is there someone else who has the necessary JavaScript runtime expertise that should look at this?

[youenn fablet]

(In reply to comment #24)
> Comment on attachment 259611 [details]
> Trying to fix mac build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259611&action=review
> 
> >>> Source/JavaScriptCore/builtins/ReadableStream.js:75
> >>> +    this.@highWaterMark = Number(strategy.highWaterMark);
> >> 
> >> Is this really the right thing to do for the number? Does this unnecessarily box the number? Maybe we should be using "+ 0" or some other idiom instead.
> > 
> > The spec mandates it.
> > We would probably loose the case of HWM set to "Infinity".

I was meaning "Infinity" as a string, not as a number.

> No, we wouldn’t lose infinity. You can add zero to infinity and you will get
> infinity. To get this right it would be better to understand clearly what
> the spec mandates.

Spec says "Set highWaterMark to ToNumber(highWaterMark)", ToNumber being defined as http://www.ecma-international.org/ecma-262/6.0/index.html#sec-tonumber

[Darin Adler]

(In reply to comment #26)
> Spec says "Set highWaterMark to ToNumber(highWaterMark)", ToNumber being
> defined as
> http://www.ecma-international.org/ecma-262/6.0/index.html#sec-tonumber

After looking over some ECMAScript documentation, I’m pretty sure that a good efficient way to write ToNumber(x) in JavaScript is +x. I am pretty sure that Number(x) does something different and likely considerably less efficient.

[youenn fablet]

(In reply to comment #27)
> (In reply to comment #26)
> > Spec says "Set highWaterMark to ToNumber(highWaterMark)", ToNumber being
> > defined as
> > http://www.ecma-international.org/ecma-262/6.0/index.html#sec-tonumber
> 
> After looking over some ECMAScript documentation, I’m pretty sure that a
> good efficient way to write ToNumber(x) in JavaScript is +x. I am pretty
> sure that Number(x) does something different and likely considerably less
> efficient.

I may be missing something but calling Number() as a function should be functionally equivalent.
+x is shorter though.

[youenn fablet]

> I may be missing something but calling Number() as a function should be
> functionally equivalent.

Meaning that it does only a call to ToNumber.

[Yusuke Suzuki]

After discussing with Geoff, we think implementing DOM objects in JSC side is not good approach. Streams should be implemented in WebCore.

Implementing Streams in JS itself looks quite reasonable to me. Streams involve callbacks and it makes managing reference counts harder.
I think we need some way to implement DOM object in JS outside JSC.

[youenn fablet]

(In reply to comment #30)
> After discussing with Geoff, we think implementing DOM objects in JSC side
> is not good approach. Streams should be implemented in WebCore.

OK.
I still think streams should go to JSC in the long term.

> Implementing Streams in JS itself looks quite reasonable to me. Streams
> involve callbacks and it makes managing reference counts harder.
> I think we need some way to implement DOM object in JS outside JSC.

I thought about it a while ago and agree it would be nice to have it, not only for streams. The deprecated webkitGetUserMedia for instance could be implemented in JS for instance.

I will have a look at it, as part of bug 147556 (ReadableStream::pipeThrough implementation).
Any advice appreciated.

[youenn fablet]

Created attachment 262255 [details]
Patch

[youenn fablet]

Created attachment 262325 [details]
Minor refactoring

[WebKit Commit Bot]

Attachment 262325 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:73:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 262428 [details]
Refactoring constructor for future reuse

[WebKit Commit Bot]

Attachment 262428 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMConstructor.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 262428 [details]
Refactoring constructor for future reuse

Clearing flags on attachment: 262428

Committed r190608: <http://trac.webkit.org/changeset/190608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.