Bug 151631 – [Streams API] Directly use @then as much as possible

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Directly using promise.@then() in lieu of @Promise.prototype.@then.@call() is more readable.

[youenn fablet]

Created attachment 266195 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 266195 [details]
Patch

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

Patch is an improvement and it improves code readability. My only concern is that it creates a duality when vending some promises as some are still done thru the prototype.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:99
> -    @Promise.prototype.@catch.@call(reader.closed, function(e) {
> +    reader.@closedPromiseCapability.@promise.@catch(function(e) {

I guess this doesn't create any brand check issues in the tests, right?

> Source/WebCore/Modules/streams/StreamInternals.js:40
> +    const promise = @Promise.@resolve(method.@apply(object, args));
> +    if (typeof promise.@then === "undefined")
> +        promise.@then = @Promise.prototype.@then;
> +    return promise;

We might want to refactor this into a function.

> Source/WebCore/Modules/streams/StreamInternals.js:67
> +        const promise = @Promise.@resolve(method.@apply(object, args1));
> +        if (typeof promise.@then === "undefined")
> +            promise.@then = @Promise.prototype.@then;
> +        return promise;

Ditto.

[youenn fablet]

Thanks for the feedback.

> Patch is an improvement and it improves code readability. My only concern is
> that it creates a duality when vending some promises as some are still done
> thru the prototype.

I am not sure which duality you are mentioning.

It is true that we will be calling @then as a property for promises whose prototype was changed. For all other promises, we will call the same @then function as a property of the prototype. Since this is the same function, this seems ok to me.

The @Promise.prototype.@then.@call pattern is replaced in all cases except pipeTo.
Once pipeTo will use abstract stream operations, this pattern should also be replaced there.
This will end that "duality".

> 
> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:99
> > -    @Promise.prototype.@catch.@call(reader.closed, function(e) {
> > +    reader.@closedPromiseCapability.@promise.@catch(function(e) {
> 
> I guess this doesn't create any brand check issues in the tests, right?

reader is always a ReadableStreamReader instance in that case so no brand check issue.
Somehow this is an optimization.

The change from reader.closed to reader.@... is due to the fact that closed property of ReadableStreamReader prototype may be changed by user scripts.  

reader being not exposed to user script, reader.@closedPromiseCapability.@promise is also not exposed.
Hence why we are sure @catch property is available (although we should not use it as per bug 151625).

> > Source/WebCore/Modules/streams/StreamInternals.js:40
> > +    const promise = @Promise.@resolve(method.@apply(object, args));
> > +    if (typeof promise.@then === "undefined")
> > +        promise.@then = @Promise.prototype.@then;
> > +    return promise;
> 
> We might want to refactor this into a function.

OK.
I can do that as a later patch or at landing.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> > Patch is an improvement and it improves code readability. My only concern is
> > that it creates a duality when vending some promises as some are still done
> > thru the prototype.
> 
> I am not sure which duality you are mentioning.

The duality I refer to is that so far we had only one way to call "then" at the code, which was using the prototype. Now we have two, which is not necessarily bad, but for people reading the code it could be weird to now where that then comes from.

> It is true that we will be calling @then as a property for promises whose
> prototype was changed. For all other promises, we will call the same @then
> function as a property of the prototype. Since this is the same function,
> this seems ok to me.
> 
> The @Promise.prototype.@then.@call pattern is replaced in all cases except
> pipeTo.
> Once pipeTo will use abstract stream operations, this pattern should also be
> replaced there.
> This will end that "duality".

Good.

> > 
> > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:99
> > > -    @Promise.prototype.@catch.@call(reader.closed, function(e) {
> > > +    reader.@closedPromiseCapability.@promise.@catch(function(e) {
> > 
> > I guess this doesn't create any brand check issues in the tests, right?
> 
> reader is always a ReadableStreamReader instance in that case so no brand
> check issue.
> Somehow this is an optimization.

Good.

> The change from reader.closed to reader.@... is due to the fact that closed
> property of ReadableStreamReader prototype may be changed by user scripts.  

I understand.

I have one big concern about all these changes. Spec speaks about creating and using promises. I don't know if the possibility of the user changing the prototype or constructor and screwing things should be avoided. Saying it other way, maybe we are being too paternalistic here when interpreting the spec and se should be more literal. This is just one big doubt. I am not sure at all about it :)

> > > Source/WebCore/Modules/streams/StreamInternals.js:40
> > > +    const promise = @Promise.@resolve(method.@apply(object, args));
> > > +    if (typeof promise.@then === "undefined")
> > > +        promise.@then = @Promise.prototype.@then;
> > > +    return promise;
> > 
> > We might want to refactor this into a function.
> 
> OK.
> I can do that as a later patch or at landing.

Awesome. Thanks!

[youenn fablet]

> I have one big concern about all these changes. Spec speaks about creating
> and using promises. I don't know if the possibility of the user changing the
> prototype or constructor and screwing things should be avoided. Saying it
> other way, maybe we are being too paternalistic here when interpreting the
> spec and se should be more literal. This is just one big doubt. I am not
> sure at all about it :)

In general, it seems to me that an implementation choice (C++ or JS built-in) should not change the behavior visible from user scripts.
Also, this is not only about screwing. It is also about ensuring private data does not leak to web pages by some nasty prototype tweaking.

[youenn fablet]

Created attachment 266352 [details]
Rebased patch

[youenn fablet]

(In reply to comment #6)
> Created attachment 266352 [details]
> Rebased patch

Rebased according other streams patches, added "shieldingPromiseResolve" helper function and migrated some "typeof xx === 'undefined'" to xx === undefined.

[youenn fablet]

(In reply to comment #7)
> (In reply to comment #6)
> > Created attachment 266352 [details]
> > Rebased patch
> 
> Rebased according other streams patches, added "shieldingPromiseResolve"
> helper function and migrated some "typeof xx === 'undefined'" to xx ===
> undefined.

Ping review?

[WebKit Commit Bot]

Comment on attachment 266352 [details]
Rebased patch

Clearing flags on attachment: 266352

Committed r194035: <http://trac.webkit.org/changeset/194035>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.