Bug 150835 – [Streams API] Vended promise capabilities should not need @resolve/@reject fields

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Following on bug 150627, vended promise capabilities in the streams API should not need @resolve/@reject fields.
I also suspect we could do clean-up as well in the promise implementation, as a GC-friendly step.

[youenn fablet]

Created attachment 264683 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 264683 [details]
Patch

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

> Source/WebCore/Modules/streams/WritableStreamInternals.js:52
> +    const shouldApplyBackpressure = [e-mail] > [e-mail];
> +    if (shouldApplyBackpressure && stream.@state === @streamWritable) {
>          stream.@state = @streamWaiting;
>          stream.@readyPromiseCapability = @newPromiseCapability(Promise);
> -    } else {
> +    }
> +    if (!shouldApplyBackpressure && stream.@state === @streamWaiting) {

I personally dislike this option. I think it was better to use the functions to create the vended promises because we are closer to the spec and it is less error prone in case we have to change anything. Instead of this patch I would have functions to create vended promise capabilities as I did in earlier versions of patch for bug 150627.

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 264683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264683&action=review
> 
> > Source/WebCore/Modules/streams/WritableStreamInternals.js:52
> > +    const shouldApplyBackpressure = [e-mail] > [e-mail];
> > +    if (shouldApplyBackpressure && stream.@state === @streamWritable) {
> >          stream.@state = @streamWaiting;
> >          stream.@readyPromiseCapability = @newPromiseCapability(Promise);
> > -    } else {
> > +    }
> > +    if (!shouldApplyBackpressure && stream.@state === @streamWaiting) {
> 
> I personally dislike this option. I think it was better to use the functions
> to create the vended promises because we are closer to the spec and it is
> less error prone in case we have to change anything. Instead of this patch I
> would have functions to create vended promise capabilities as I did in
> earlier versions of patch for bug 150627.

These specific changes make the implementation closer to the spec and removes the fact that we were resolving an already resolved promise.

These changes should be done no matter whether we remove @resolve/@reject for vended promises.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> These specific changes make the implementation closer to the spec and
> removes the fact that we were resolving an already resolved promise.
> 
> These changes should be done no matter whether we remove @resolve/@reject
> for vended promises.

You're right. I missed that from the spec that it was causing that the promises were vended more than once.

[WebKit Commit Bot]

Comment on attachment 264683 [details]
Patch

Clearing flags on attachment: 264683

Committed r191956: <http://trac.webkit.org/changeset/191956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.