Bug 166752 – Handle IDLPromise<> properly

• Component: Bindings
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Sam Weinig]

Any thoughts on a design for this, I have been struggling a bit. 

The biggest issue is that unlike all other types, the way our promise code works, you always have to pass a JSDOMGlobalObject and an ExecState to any function that might want to create a promise. Ideally, what we would do would be to have a new type that can be instantiated without a JSDOMGlobalObject and an ExecState, that gets wrapped by the JS promise once it enters the bindings. e.g.

Ref<NonJSPromise> MyClass::load()
{
    ...

    m_promise = NonJSPromise::create();

    ...

    return m_promise.releaseNonNull();
}

And then the bindings layer does something that creates a JSPromiseDeferred from the NonJSPromise, but I am not clear on how the lifetime of the JSPromiseDeferred would be managed. Perhaps the NonJSPromise (now even more poorly named) could act as a lazy WebCore::DeferredPromise, and fill in the JSPromiseDeferred and JSDOMGlobalObject when it gets passed into the bindings layer?

Anyway, maybe you have a better design in mind.

[Yusuke Suzuki]

Created attachment 307964 [details]
Patch

[Yusuke Suzuki]

Created attachment 307965 [details]
Patch

[Yusuke Suzuki]

Created attachment 308672 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 308672 [details]
Patch

Still I'm considering about the design.

[Yusuke Suzuki]

(In reply to Sam Weinig from comment #1)
> Any thoughts on a design for this, I have been struggling a bit. 
> 
> The biggest issue is that unlike all other types, the way our promise code
> works, you always have to pass a JSDOMGlobalObject and an ExecState to any
> function that might want to create a promise. Ideally, what we would do
> would be to have a new type that can be instantiated without a
> JSDOMGlobalObject and an ExecState, that gets wrapped by the JS promise once
> it enters the bindings. e.g.
> 
> Ref<NonJSPromise> MyClass::load()
> {
>     ...
> 
>     m_promise = NonJSPromise::create();
> 
>     ...
> 
>     return m_promise.releaseNonNull();
> }
> 
> And then the bindings layer does something that creates a JSPromiseDeferred
> from the NonJSPromise, but I am not clear on how the lifetime of the
> JSPromiseDeferred would be managed. Perhaps the NonJSPromise (now even more
> poorly named) could act as a lazy WebCore::DeferredPromise, and fill in the
> JSPromiseDeferred and JSDOMGlobalObject when it gets passed into the
> bindings layer?
> 
> Anyway, maybe you have a better design in mind.

Is there any case that poses promise<> as an attribute and that promise<> does not come from userspace?
Currently, PromiseRejectionEvent always takes promise from user-space. Thus, the promise is always JSPromise thing.
So we have no way other than holding it in the event object in some manner since users can observe the promise identity by `event.promise === passedPromise`.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> (In reply to Sam Weinig from comment #1)
> > Any thoughts on a design for this, I have been struggling a bit. 
> > 
> > The biggest issue is that unlike all other types, the way our promise code
> > works, you always have to pass a JSDOMGlobalObject and an ExecState to any
> > function that might want to create a promise. Ideally, what we would do
> > would be to have a new type that can be instantiated without a
> > JSDOMGlobalObject and an ExecState, that gets wrapped by the JS promise once
> > it enters the bindings. e.g.
> > 
> > Ref<NonJSPromise> MyClass::load()
> > {
> >     ...
> > 
> >     m_promise = NonJSPromise::create();
> > 
> >     ...
> > 
> >     return m_promise.releaseNonNull();
> > }
> > 
> > And then the bindings layer does something that creates a JSPromiseDeferred
> > from the NonJSPromise, but I am not clear on how the lifetime of the
> > JSPromiseDeferred would be managed. Perhaps the NonJSPromise (now even more
> > poorly named) could act as a lazy WebCore::DeferredPromise, and fill in the
> > JSPromiseDeferred and JSDOMGlobalObject when it gets passed into the
> > bindings layer?
> > 
> > Anyway, maybe you have a better design in mind.
> 
> Is there any case that poses promise<> as an attribute and that promise<>
> does not come from userspace?
> Currently, PromiseRejectionEvent always takes promise from user-space. Thus,
> the promise is always JSPromise thing.
> So we have no way other than holding it in the event object in some manner
> since users can observe the promise identity by `event.promise ===
> passedPromise`.

Currently, I'm considering about just using Ref<DOMGuarded<JSPromise>> for PromiseRejectionEvent.
Strong<JSPromise> causes cyclic reference if the wrapper has a ref to the promise and promise has a ref to the wrapper.

BTW, currently the PromiseRejectionEvent holds reason as Strong<JSC::Unknown>. It is also not good. (And CustomEvent::detail too.

[Yusuke Suzuki]

Created attachment 308693 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 308693 [details]
Patch

I'll do a bit more work.

[Yusuke Suzuki]

Created attachment 308714 [details]
Patch

[Sam Weinig]

Does this allow us to remove any of the custom bindings that only exist for promises (e.g. JSFontFaceCustom.cpp, JSFontFaceSetCustom.cpp, JSMediaKeySessionCustom.cpp, and JSWebGPUCommandBuffer.cpp)?

[Yusuke Suzuki]

(In reply to Sam Weinig from comment #11)
> Does this allow us to remove any of the custom bindings that only exist for
> promises (e.g. JSFontFaceCustom.cpp, JSFontFaceSetCustom.cpp,
> JSMediaKeySessionCustom.cpp, and JSWebGPUCommandBuffer.cpp)?

This patch does nothing right now for these things. It is now focusing on PromiseRejectionEvent's promise, which is super tricky because `attribute promise<any>`.

On the other hand, JSFontFaceCustom.cpp etc.'s promise definitions are like

attribute Promise<IDLInterface|void>

And there is no Promise<any> thing. In that case, we can implement it easier.
We can implement these thing as you suggested. I'll implement it in a separate patch.

[Yusuke Suzuki]

Created attachment 309038 [details]
Patch

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #12)
> (In reply to Sam Weinig from comment #11)
> > Does this allow us to remove any of the custom bindings that only exist for
> > promises (e.g. JSFontFaceCustom.cpp, JSFontFaceSetCustom.cpp,
> > JSMediaKeySessionCustom.cpp, and JSWebGPUCommandBuffer.cpp)?
> 
> This patch does nothing right now for these things. It is now focusing on
> PromiseRejectionEvent's promise, which is super tricky because `attribute
> promise<any>`.
> 
> On the other hand, JSFontFaceCustom.cpp etc.'s promise definitions are like
> 
> attribute Promise<IDLInterface|void>
> 
> And there is no Promise<any> thing. In that case, we can implement it easier.
> We can implement these thing as you suggested. I'll implement it in a
> separate patch.

👍

[Yusuke Suzuki]

Rebasing the patch right now...

[Yusuke Suzuki]

Created attachment 309087 [details]
Patch

[Build Bot]

Comment on attachment 309087 [details]
Patch

Attachment 309087 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/3674137

New failing tests:
(JS) JSTestPromiseRejectionEvent.cpp
(JS) JSTestPromiseRejectionEvent.h

[Yusuke Suzuki]

Created attachment 309089 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 309038 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        In conversion phase, we just convert the value to JSC::JSPromise* and hold it in
> +        PromiseRejectionEvent. On the other hand, In this patch, we newly introduce a new
> +        type DOMPromise and use it in RejectedPromiseTracker. And we also rename the previous
> +        DOMPromise to DOMPromiseDeferred since it is corresponding to JSPromiseDeferred.
> +        DOMPromise is DOMGuarded object. So it is strongly referenced from ScriptExecutionContext
> +        and it is weakly referenced from the object itself. This is important since Strong<JSPromise>
> +        reference in C++ object that has a wrapper (in this case, PromiseRejectionEvent) easily causes
> +        cyclic reference. We hold it as DOMPromise instead of Strong<JSPromise> in RejectedPromiseTracker
> +        to break the cyclic reference edge with weak reference.

I'm still lost on all of the different types, but at least the list appears consistent now.

Here is how I understand them, please correct me if I'm wrong:

    JSC::JSPromise
    JSC::JSPromiseDeferred
      - Native Promise JSValue instances

    JSC::JSInternalPromise
    JSC::JSInternalPromiseDeferred
      - A separate implementation of Promises that has no web exposed overridable APIs
      - Safe to use internally without the possibility of user overridden behavior anywhere
      - Used only for module loading right now, but presumably any internal feature could use them

    DOMPromise
    DOMPromiseDeferred
      - Wrapper around a Native Promise JSValue instances
      - Attempts to tie lifetime to a JSGlobalObject (ScriptExecutionContext)

Could you clarify what the difference is between a Promise and a PromiseDeferred? Is a Deferred just a Promise with its Resolve + Reject functions so that C++ can resolve/reject the promise if needed?

----

I'm also confused by DOMGuarded in general. There should really be some short comment in that class to describe its use case!

It seems like it is just JSC::Weak that is also an ActiveDOMCallback so it is aware of whether or not a ScriptExecutionContext is suspended or not. Can its GlobalObject ever be different from its ScriptExecutionContext?

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:51
> +        if (scope.exception()) {
> +            exceptionThrower(state, scope);

Is there a test we can write for this exception case or is it only possible for cases like exhausted stack / out of memory?

[Yusuke Suzuki]

Comment on attachment 309038 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        to break the cyclic reference edge with weak reference.
> 
> I'm still lost on all of the different types, but at least the list appears consistent now.
> 
> Here is how I understand them, please correct me if I'm wrong:
> 
>     JSC::JSPromise
>     JSC::JSPromiseDeferred
>       - Native Promise JSValue instances
> 
>     JSC::JSInternalPromise
>     JSC::JSInternalPromiseDeferred
>       - A separate implementation of Promises that has no web exposed overridable APIs
>       - Safe to use internally without the possibility of user overridden behavior anywhere
>       - Used only for module loading right now, but presumably any internal feature could use them
> 
>     DOMPromise
>     DOMPromiseDeferred
>       - Wrapper around a Native Promise JSValue instances
>       - Attempts to tie lifetime to a JSGlobalObject (ScriptExecutionContext)
> 
> Could you clarify what the difference is between a Promise and a PromiseDeferred? Is a Deferred just a Promise with its Resolve + Reject functions so that C++ can resolve/reject the promise if needed?
> 
> ----
> 
> I'm also confused by DOMGuarded in general. There should really be some short comment in that class to describe its use case!
> 
> It seems like it is just JSC::Weak that is also an ActiveDOMCallback so it is aware of whether or not a ScriptExecutionContext is suspended or not. Can its GlobalObject ever be different from its ScriptExecutionContext?

> Is a Deferred just a Promise with its Resolve + Reject functions so that C++ can resolve/reject the promise if needed?

Right. Deferred is the name conventionally used for the tuple of (promise, resolve, reject). Promise is the abstraction of the future value.
And it does not offer the way to resolve / reject itself. Deferred's resolve / reject functions are the way to do this.
This is a good separation: API will expose promise, but won't expose resolve and reject functions.

> It seems like it is just JSC::Weak that is also an ActiveDOMCallback so it is aware of whether or not a ScriptExecutionContext is suspended or not. Can its GlobalObject ever be different from its ScriptExecutionContext?

No. DOMGuarded's constructor uses ScriptExecutionContext of the given GlobalObject. And that GlobalObject will be never replaced.
It is a bit special function. In WebCore, many APIs tied to ScriptExecutionContext will resolve / reject exposed promises. CustomElementRegistry's promise is one example.
But if we hold it Strong<>, it easily causes cyclic reference... So we should have a way to break this reference cycle. DOMPromiseDeferred / DOMPromise are things.
It is Weak reference, but it is referenced by ScriptExecutionContext. So it is live during (1) Ref<DOMPromise> is live and (2) ScriptExecutionContext is live.

Since these promises are never shown from the DOM objects once ScriptExecutionContext is gone, this wrapper works super well.

>> Source/WebCore/bindings/js/JSDOMConvertPromise.h:51
>> +            exceptionThrower(state, scope);
> 
> Is there a test we can write for this exception case or is it only possible for cases like exhausted stack / out of memory?

Right. It only happens if the stack is exhausted.

In a long term, I'm planning to clean up the current Promise implementation.
I think we should have 2 ways to construct Promises. Normal JS way, and C++ way. Both should be super fast. And in C++ way,
we should not create resolve / reject callbacks. At that time, the above function should not call JS functions. And this check will be eliminated.

[Yusuke Suzuki]

Created attachment 309098 [details]
Patch

Fix debug build

[youenn fablet]

I am not sure whether DOMPromise is more aligned with JSPromiseDeferred or JSPromise.
Is it the case? If so, why?
If it is more aligned with JSPromiseDeferred, I am fine with this change.

If I were to implement PromiseRejectionEvent, I would probably use JS builtin to accomodate for the fact that the promise passed value may either be a promise or a value that would be promise-resolved.
Can you describe what is the advantage of your approach?

[Yusuke Suzuki]

(In reply to youenn fablet from comment #22)
> I am not sure whether DOMPromise is more aligned with JSPromiseDeferred or
> JSPromise.
> Is it the case? If so, why?
> If it is more aligned with JSPromiseDeferred, I am fine with this change.

Newly introduced DOMPromise is aligned to JSPromise.
Previous DOMPromise, which is now renamed to DOMPromiseDeferred, is aligned to JSPromiseDeferred.

> If I were to implement PromiseRejectionEvent, I would probably use JS
> builtin to accomodate for the fact that the promise passed value may either
> be a promise or a value that would be promise-resolved.
> Can you describe what is the advantage of your approach?

IDL code generator can emit the conversion code instead of writing it.

[youenn fablet]

(In reply to Yusuke Suzuki from comment #23)
> (In reply to youenn fablet from comment #22)
> > I am not sure whether DOMPromise is more aligned with JSPromiseDeferred or
> > JSPromise.
> > Is it the case? If so, why?
> > If it is more aligned with JSPromiseDeferred, I am fine with this change.
> 
> Newly introduced DOMPromise is aligned to JSPromise.
> Previous DOMPromise, which is now renamed to DOMPromiseDeferred, is aligned
> to JSPromiseDeferred.

Right, it makes sense to rename DOMPromise in DOMPromiseDeferred.

There may be a small risk of confusion by having both DOMPromise and DOMPromiseDeferred in the code base though.
I guess the vast majority of DOM C++ code will stick to DOMPromiseDeferred since they will want to resolve/reject the promise.

> > If I were to implement PromiseRejectionEvent, I would probably use JS
> > builtin to accomodate for the fact that the promise passed value may either
> > be a promise or a value that would be promise-resolved.
> > Can you describe what is the advantage of your approach?
> 
> IDL code generator can emit the conversion code instead of writing it.

If RejectedPromiseEvent is the only case of DOMPromise use, I would tend to stick with JS built-in, since this introduces less code.
If we envision other usages, it seems fine to introduce DOMPromise.

[youenn fablet]

(In reply to Sam Weinig from comment #1)
> Any thoughts on a design for this, I have been struggling a bit. 
> 
> The biggest issue is that unlike all other types, the way our promise code
> works, you always have to pass a JSDOMGlobalObject and an ExecState to any
> function that might want to create a promise. Ideally, what we would do
> would be to have a new type that can be instantiated without a
> JSDOMGlobalObject and an ExecState, that gets wrapped by the JS promise once
> it enters the bindings. e.g.
> 
> Ref<NonJSPromise> MyClass::load()
> {
>     ...
> 
>     m_promise = NonJSPromise::create();
> 
>     ...
> 
>     return m_promise.releaseNonNull();
> }
> 
> And then the bindings layer does something that creates a JSPromiseDeferred
> from the NonJSPromise, but I am not clear on how the lifetime of the
> JSPromiseDeferred would be managed. Perhaps the NonJSPromise (now even more
> poorly named) could act as a lazy WebCore::DeferredPromise, and fill in the
> JSPromiseDeferred and JSDOMGlobalObject when it gets passed into the
> bindings layer?
> 
> Anyway, maybe you have a better design in mind.

Yep, that would be great to have that.
I think this is how Chromium is doing and the code looks nicer.

[Yusuke Suzuki]

(In reply to youenn fablet from comment #24)
> (In reply to Yusuke Suzuki from comment #23)
> > (In reply to youenn fablet from comment #22)
> > > I am not sure whether DOMPromise is more aligned with JSPromiseDeferred or
> > > JSPromise.
> > > Is it the case? If so, why?
> > > If it is more aligned with JSPromiseDeferred, I am fine with this change.
> > 
> > Newly introduced DOMPromise is aligned to JSPromise.
> > Previous DOMPromise, which is now renamed to DOMPromiseDeferred, is aligned
> > to JSPromiseDeferred.
> 
> Right, it makes sense to rename DOMPromise in DOMPromiseDeferred.
> 
> There may be a small risk of confusion by having both DOMPromise and
> DOMPromiseDeferred in the code base though.
> I guess the vast majority of DOM C++ code will stick to DOMPromiseDeferred
> since they will want to resolve/reject the promise.

Yeah. I think it's ok because DOMPromsie does not have resolve / reject functions.
Its interface tells us that DOMPromise is not resolvable one.
Thus, users will hold DOMPromiseDeferred if users need to resolve / reject it.
I think holding promises as DOMPromise in RejectedPromiseTracker is good since RejectedPromiseTracker's event emission lifetime is tied to ScriptExecutionContext. Thus, current DOMPromise / DOMPromiseDeferred lifetime is completely matched against this.

> 
> > > If I were to implement PromiseRejectionEvent, I would probably use JS
> > > builtin to accomodate for the fact that the promise passed value may either
> > > be a promise or a value that would be promise-resolved.
> > > Can you describe what is the advantage of your approach?
> > 
> > IDL code generator can emit the conversion code instead of writing it.
> 
> If RejectedPromiseEvent is the only case of DOMPromise use, I would tend to
> stick with JS built-in, since this introduces less code.
> If we envision other usages, it seems fine to introduce DOMPromise.

You mean using JSBuiltinConstructor, and private method to store promise value to C++ PromiseRejectionEvent, correct? If so, I agree with you.

[youenn fablet]

> You mean using JSBuiltinConstructor, and private method to store promise
> value to C++ PromiseRejectionEvent, correct? If so, I agree with you.

Yes

[Yusuke Suzuki]

(In reply to youenn fablet from comment #27)
> > You mean using JSBuiltinConstructor, and private method to store promise
> > value to C++ PromiseRejectionEvent, correct? If so, I agree with you.
> 
> Yes

I've a bit prototyped this, and I think the current design is rather simple.
I'll describe why I think so.

1. PromiseRejectionEvent is derived class of Event.

While FetchReponse etc. is not derived class from any existing DOM interface, PromiseRejectionEvent is derived class of Event. When creating Event, we need AtomicString type parameter. So, the current JSBuiltinConstructor design is not suitable for that. Of course, we can change Event not to get type string in its constructor (and add a setter to set type string later), but I think it is not good design since we always assume that Event should have type string.

2. Not reduce the code

If we use JSBulitinConstructor, we additionally need,

(1) PromiseRejectionEvent.js, which should have initializePromiseRejectionEvent() and its code.
(2) initializeWith(any, promise<any>) private function to fill the fields from promise rejection event.

I think it increases the code rather than introducing DOM conversion. Current PromiseRejectionEvent constructor is well handled in DOM binding IDL: event init dictionary is automatically extracted by IDL generator. So only required thing is DOM conversion part.

What do you think of?

[youenn fablet]

> 1. PromiseRejectionEvent is derived class of Event.
> 
> While FetchReponse etc. is not derived class from any existing DOM
> interface, PromiseRejectionEvent is derived class of Event. When creating
> Event, we need AtomicString type parameter. So, the current
> JSBuiltinConstructor design is not suitable for that. Of course, we can
> change Event not to get type string in its constructor (and add a setter to
> set type string later), but I think it is not good design since we always
> assume that Event should have type string.

I see, that may be an issue indeed.


> 2. Not reduce the code
> 
> If we use JSBulitinConstructor, we additionally need,
> 
> (1) PromiseRejectionEvent.js, which should have
> initializePromiseRejectionEvent() and its code.
> (2) initializeWith(any, promise<any>) private function to fill the fields
> from promise rejection event.

The idea would be to JSBuiltin the constructor so as to add @promise/@reason fields created from values passed to the constructor in initializePromiseRejectionEvent.
Then we would JSBuiltin the promise/reason attributes to either return @promise/@reason slots if defined or call the DOM C++ getter otherwise.

> I think it increases the code rather than introducing DOM conversion.
> Current PromiseRejectionEvent constructor is well handled in DOM binding
> IDL: event init dictionary is automatically extracted by IDL generator. So
> only required thing is DOM conversion part.
> 
> What do you think of?

Yeah, the fact that PromiseRejectionEvent is an event and JSBuiltinConstructor does not work out of the box might indeed be an issue.

Looking at PromiseRejectionEvent, it looks really JSish at the moment.
I wonder whether using more JS builtins would help or not there in the implementation of RejectedPromiseTracker.
If so, then maybe it is worth going the JSBuiltinConstructor way.

I guess either way is fine.

[Yusuke Suzuki]

(In reply to youenn fablet from comment #29)
> > 1. PromiseRejectionEvent is derived class of Event.
> > 
> > While FetchReponse etc. is not derived class from any existing DOM
> > interface, PromiseRejectionEvent is derived class of Event. When creating
> > Event, we need AtomicString type parameter. So, the current
> > JSBuiltinConstructor design is not suitable for that. Of course, we can
> > change Event not to get type string in its constructor (and add a setter to
> > set type string later), but I think it is not good design since we always
> > assume that Event should have type string.
> 
> I see, that may be an issue indeed.
> 
> 
> > 2. Not reduce the code
> > 
> > If we use JSBulitinConstructor, we additionally need,
> > 
> > (1) PromiseRejectionEvent.js, which should have
> > initializePromiseRejectionEvent() and its code.
> > (2) initializeWith(any, promise<any>) private function to fill the fields
> > from promise rejection event.
> 
> The idea would be to JSBuiltin the constructor so as to add @promise/@reason
> fields created from values passed to the constructor in
> initializePromiseRejectionEvent.
> Then we would JSBuiltin the promise/reason attributes to either return
> @promise/@reason slots if defined or call the DOM C++ getter otherwise.

What happens if the wrapper object is gone?
I think the problem is that PromiseRejectionEvent is derived class of Event.
And we usually use Event, Ref<Event> and RefPtr<Event> in WebCore code. So, I think wrapper can be gone.
When the wrapper is gone, we need to retrieve reason and promise from C++ PromiseRejectionEvent. So anyway, we need to call `initializeWith(any, promise<any>)` private function to fill the C++ fields in initializePromiseRejectionEvent.

> 
> > I think it increases the code rather than introducing DOM conversion.
> > Current PromiseRejectionEvent constructor is well handled in DOM binding
> > IDL: event init dictionary is automatically extracted by IDL generator. So
> > only required thing is DOM conversion part.
> > 
> > What do you think of?
> 
> Yeah, the fact that PromiseRejectionEvent is an event and
> JSBuiltinConstructor does not work out of the box might indeed be an issue.
> 
> Looking at PromiseRejectionEvent, it looks really JSish at the moment.
> I wonder whether using more JS builtins would help or not there in the
> implementation of RejectedPromiseTracker.

I agree with you about this thing. RejectedPromiseTracker can be implemented in JSC world, and I think, in the future, we should do that.
It will offer the way to handle unhandled-rejected-promises even in JSC shell. (And it is very nice to debug async tests in JSC shell).

> If so, then maybe it is worth going the JSBuiltinConstructor way.

But when doing that, maybe, we will just hold the promise, and we do not create event in JS side since JSC does not have the notion of events.
If I implement it, I'll merge large part of RejectedPromiseTracker into JSC, and JSC will invoke handlers with the reason and promises.
And WebCore sets handlers to JSC, and WebCore will create events based on the passed rejected promises. And dispatch events.

> 
> I guess either way is fine.

So, for PromiseRejectionEvent, I think the current design is OK.

[youenn fablet]

Comment on attachment 309098 [details]
Patch

OK, let's go!
Some nits below.

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

> Source/JavaScriptCore/runtime/JSPromise.cpp:99
> +    auto scope = DECLARE_THROW_SCOPE(vm);

maybe use auto for all 3 and below as well?

> Source/JavaScriptCore/runtime/JSPromise.h:55
> +    JS_EXPORT_PRIVATE static JSPromise* resolve(JSGlobalObject*, JSValue);

Shouldn't we try to pass a JSGlobalObject&?

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:43
> +        JSDOMGlobalObject* globalObject = jsDynamicDowncast<JSDOMGlobalObject*>(vm, state.lexicalGlobalObject());

auto*

> Source/WebCore/bindings/js/JSDOMConvertPromise.h:49
> +        JSC::JSPromise* promise = JSC::JSPromise::resolve(globalObject, value);

Ditto

> Source/WebCore/dom/RejectedPromiseTracker.cpp:53
>  class RejectedPromise {

Do we still need RejectedPromise?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:125
> +void RejectedPromiseTracker::promiseHandled(ExecState&, JSDOMGlobalObject& globalObject, JSPromise& promise)

Change signature in a follow-up?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:167
> +        Ref<DOMPromise> domPromise = unhandledPromise.promise();

Why do we need to take a Ref here?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:170
> +        ExecState& state = *domPromise->globalObject()->globalExec();

auto?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:200
> +    Ref<DOMPromise> domPromise = rejectedPromise.promise();

Why taking a ref here?

> Source/WebCore/dom/RejectedPromiseTracker.cpp:209
> +    initializer.reason = promise.result(vm);

Brace initializer maybe?

[Yusuke Suzuki]

Comment on attachment 309098 [details]
Patch

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

Thanks for your review!

>> Source/JavaScriptCore/runtime/JSPromise.cpp:99
>> +    auto scope = DECLARE_THROW_SCOPE(vm);
> 
> maybe use auto for all 3 and below as well?

Fixed.

>> Source/JavaScriptCore/runtime/JSPromise.h:55
>> +    JS_EXPORT_PRIVATE static JSPromise* resolve(JSGlobalObject*, JSValue);
> 
> Shouldn't we try to pass a JSGlobalObject&?

Changed.

>> Source/WebCore/bindings/js/JSDOMConvertPromise.h:43
>> +        JSDOMGlobalObject* globalObject = jsDynamicDowncast<JSDOMGlobalObject*>(vm, state.lexicalGlobalObject());
> 
> auto*

Changed.

>> Source/WebCore/bindings/js/JSDOMConvertPromise.h:49
>> +        JSC::JSPromise* promise = JSC::JSPromise::resolve(globalObject, value);
> 
> Ditto

Changed.

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:53
>>  class RejectedPromise {
> 
> Do we still need RejectedPromise?

Yeah, we can drop this. Dropped.

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:125
>> +void RejectedPromiseTracker::promiseHandled(ExecState&, JSDOMGlobalObject& globalObject, JSPromise& promise)
> 
> Change signature in a follow-up?

That sound fine! Filed. https://bugs.webkit.org/show_bug.cgi?id=171856

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:167
>> +        Ref<DOMPromise> domPromise = unhandledPromise.promise();
> 
> Why do we need to take a Ref here?

Right, we do not need to take it. Changed it to auto&.

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:170
>> +        ExecState& state = *domPromise->globalObject()->globalExec();
> 
> auto?

Changed.

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:200
>> +    Ref<DOMPromise> domPromise = rejectedPromise.promise();
> 
> Why taking a ref here?

Yeah, fixed.

>> Source/WebCore/dom/RejectedPromiseTracker.cpp:209
>> +    initializer.reason = promise.result(vm);
> 
> Brace initializer maybe?

PromiseRejectionEvent::Init inherits Event::Init, which has default initializers for members.
This disables brace initializer here.

[Yusuke Suzuki]

Created attachment 309489 [details]
Patch for landing

[Yusuke Suzuki]

Committed r216501: <http://trac.webkit.org/changeset/216501>

[Yusuke Suzuki]

Committed r216503: <http://trac.webkit.org/changeset/216503>

[Yusuke Suzuki]

Committed r216507: <http://trac.webkit.org/changeset/216507>