Bug 150133 – [Streams API] Rework some readable stream internals that can be common to writable streams

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[Streams API] Rework some readable stream internals that can be common to writable streams

[Xabier Rodríguez Calvar]

Created attachment 263095 [details]
Patch

This patch will need to be rebased against bug 149951 or viceversa as they will conflict during landing.

[Xabier Rodríguez Calvar]

(In reply to comment #1)
> Created attachment 263095 [details]
> Patch

I could split the patch in smaller pieces, but I think the size and complexity are manageable.

[youenn fablet]

Comment on attachment 263095 [details]
Patch

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

Sounds mostly good to me, some questions below.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:505
> +        GlobalPropertyInfo(vm.propertyNames->EvalErrorPrivateName, m_evalErrorConstructor.get(), DontEnum | DontDelete | ReadOnly),

What is the reason for adding this one?

> Source/WebCore/Modules/streams/ReadableStream.js:55
> +    this.@strategy = @validateAndNormalizeQueuingStrategy(strategy.size, strategy.highWaterMark);

Sounds very good.

> Source/WebCore/Modules/streams/StreamInternals.js:93
> +{

This adds an if test which was not needed in the past.
If implemented in C++, this could well be set as "inline".
Are these 3 promise routines useful enough to be added?

> Source/WebCore/Modules/streams/StreamInternals.js:116
> +    return { content: [], size: 0 };

Is it worth adding a function for this one?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:105
> +#undef DECLARE_GLOBAL_STATIC

We should find a way to improve this in the future.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:505
> > +        GlobalPropertyInfo(vm.propertyNames->EvalErrorPrivateName, m_evalErrorConstructor.get(), DontEnum | DontDelete | ReadOnly),
> 
> What is the reason for adding this one?

I'll use them in some later patches, where the spec just says things like "fail here" without specifying any error.

> > Source/WebCore/Modules/streams/StreamInternals.js:93
> > +{
> 
> This adds an if test which was not needed in the past.
> If implemented in C++, this could well be set as "inline".
> Are these 3 promise routines useful enough to be added?

They are useful, IMHO, otherwise I wouldn't have added them :)

I know you are adding an if clause but the idea behind it is to be able to just call resolve or reject with values like:

* ones that are not even a promise: imagine that the spec defines some case where it tells you to resolve a promise that has been unset by some earlier code flow. In that case you can still almost literal with the spec without adding any extra if clause for those cases that would create an eval error and code is easier to read and follow.
* ones created with Promise.resolve or Promise.reject, that don't have the internal slots and would create an eval error.

> > Source/WebCore/Modules/streams/StreamInternals.js:116
> > +    return { content: [], size: 0 };
> 
> Is it worth adding a function for this one?

That's a matter of taste. In the case of the promises it is more clear to me because code is more complex (keeping the internal slots and so on), but in this case I had that doubt too. I decided to add it because I think it brings coherence to the code and reduces the posibility of coding errors as you'd have to inline the same code everywhere the function is used, which not only happens when initializing, but with errors and of course also in writable stream.

> > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:105
> > +#undef DECLARE_GLOBAL_STATIC
> 
> We should find a way to improve this in the future.

I liked the way it was done with some earlier patches for bug 149751.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:505
> > > +        GlobalPropertyInfo(vm.propertyNames->EvalErrorPrivateName, m_evalErrorConstructor.get(), DontEnum | DontDelete | ReadOnly),
> > 
> > What is the reason for adding this one?
> 
> I'll use them in some later patches, where the spec just says things like
> "fail here" without specifying any error.

OK.
I would personally wait for the later patches to introduce it.

> > > Source/WebCore/Modules/streams/StreamInternals.js:93
> > > +{
> > 
> > This adds an if test which was not needed in the past.
> > If implemented in C++, this could well be set as "inline".
> > Are these 3 promise routines useful enough to be added?
> 
> They are useful, IMHO, otherwise I wouldn't have added them :)
> 
> I know you are adding an if clause but the idea behind it is to be able to
> just call resolve or reject with values like:
> 
> * ones that are not even a promise: imagine that the spec defines some case
> where it tells you to resolve a promise that has been unset by some earlier
> code flow. In that case you can still almost literal with the spec without
> adding any extra if clause for those cases that would create an eval error
> and code is easier to read and follow.
> * ones created with Promise.resolve or Promise.reject, that don't have the
> internal slots and would create an eval error.

I would tend to split promise handling refactoring in another bug.

For instance, we are using public Promise APIs (like new Promise and so on).
We may want to use internals ones.

Also, we could directly use promise operations functions like @rejectPromise or @fulfillPromise (not sure this is a good idea).

Let's see what others think.

> > > Source/WebCore/Modules/streams/StreamInternals.js:116
> > > +    return { content: [], size: 0 };
> > 
> > Is it worth adding a function for this one?
> 
> That's a matter of taste. In the case of the promises it is more clear to me
> because code is more complex (keeping the internal slots and so on), but in
> this case I had that doubt too. I decided to add it because I think it
> brings coherence to the code and reduces the posibility of coding errors as
> you'd have to inline the same code everywhere the function is used, which
> not only happens when initializing, but with errors and of course also in
> writable stream.

OK.
It would be interesting to know the kind of consequences such encapsulation has in terms of code size/performances...
As we get more js builtin code, some guidelines may be helpful.

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> > I'll use them in some later patches, where the spec just says things like
> > "fail here" without specifying any error.
> 
> OK.
> I would personally wait for the later patches to introduce it.

Yes, this is something that can be easily changed at landing time.

> > I know you are adding an if clause but the idea behind it is to be able to
> > just call resolve or reject with values like:
> > 
> > * ones that are not even a promise: imagine that the spec defines some case
> > where it tells you to resolve a promise that has been unset by some earlier
> > code flow. In that case you can still almost literal with the spec without
> > adding any extra if clause for those cases that would create an eval error
> > and code is easier to read and follow.
> > * ones created with Promise.resolve or Promise.reject, that don't have the
> > internal slots and would create an eval error.
> 
> I would tend to split promise handling refactoring in another bug.
> 
> For instance, we are using public Promise APIs (like new Promise and so on).
> We may want to use internals ones.
> 
> Also, we could directly use promise operations functions like @rejectPromise
> or @fulfillPromise (not sure this is a good idea).
> 
> Let's see what others think.

I could split it in another bug or leave it for a future follow up patch.

> > > > Source/WebCore/Modules/streams/StreamInternals.js:116
> > > > +    return { content: [], size: 0 };
> > > 
> > > Is it worth adding a function for this one?
> > 
> > That's a matter of taste. In the case of the promises it is more clear to me
> > because code is more complex (keeping the internal slots and so on), but in
> > this case I had that doubt too. I decided to add it because I think it
> > brings coherence to the code and reduces the posibility of coding errors as
> > you'd have to inline the same code everywhere the function is used, which
> > not only happens when initializing, but with errors and of course also in
> > writable stream.
> 
> OK.
> It would be interesting to know the kind of consequences such encapsulation
> has in terms of code size/performances...
> As we get more js builtin code, some guidelines may be helpful.

I don't have a strong opinion and it something that be easily done in a follow up patch.

[Xabier Rodríguez Calvar]

Created attachment 263483 [details]
Patch

Rebased against landed bug 149951 and removed EvalError as we don't plan to release anything with soon enough.

[youenn fablet]

I am still not sure with the promise refactoring.

@InternalPromise should be used.

Also the added promise routines are generic and have no streams API special bits.
If they are useful, maybe the promise built-in JS implementation could be adapted to provide these (or similar) internal APIs? yusukesuzuki, do you have an opinion on this?

That said, this can be done as a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 263483 [details]
Patch

Clearing flags on attachment: 263483

Committed r191335: <http://trac.webkit.org/changeset/191335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.