Bug 178533 – [Bindings] Standardize on DOMPromise as the way to store passed in promises

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

[Bindings] Standarize on DOMPromise as the way to store passed in promises

[Sam Weinig]

Created attachment 324270 [details]
Patch

[youenn fablet]

Comment on attachment 324270 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.h:39
> +    void updateWith(RefPtr<DOMPromise>&&);

Can we pass a Ref<DOMPromise>&& instead?
I would think nullptr to happen only in case of exception which probably means updateWith will never be called.

[youenn fablet]

Comment on attachment 324270 [details]
Patch

Looks really great!

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

> Source/WebCore/workers/service/ExtendableEvent.cpp:48
> +    addPendingPromise(*promise);

We should be able to move this one.

[Sam Weinig]

(In reply to youenn fablet from comment #2)
> Comment on attachment 324270 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324270&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.h:39
> > +    void updateWith(RefPtr<DOMPromise>&&);
> 
> Can we pass a Ref<DOMPromise>&& instead?
> I would think nullptr to happen only in case of exception which probably
> means updateWith will never be called.

The bindings don't really support that right now.

[Sam Weinig]

(In reply to Sam Weinig from comment #4)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 324270 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=324270&action=review
> > 
> > > Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.h:39
> > > +    void updateWith(RefPtr<DOMPromise>&&);
> > 
> > Can we pass a Ref<DOMPromise>&& instead?
> > I would think nullptr to happen only in case of exception which probably
> > means updateWith will never be called.
> 
> The bindings don't really support that right now.

Actually, it's easy to change that. Will fix.

[Sam Weinig]

Created attachment 324297 [details]
Patch

[youenn fablet]

Comment on attachment 324297 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.cpp:37
> +void PaymentRequestUpdateEvent::updateWith(RefPtr<DOMPromise>&&)

Ref<>?

> Source/WebCore/dom/PromiseRejectionEvent.h:38
> +        RefPtr<DOMPromise> promise;

It should probably be a Ref<DOMPromise> as promise is required.

> Source/WebCore/workers/service/FetchEvent.cpp:138
> +        [] (Ref<FormData>&) {

Indentation like for regular switch?

[Sam Weinig]

(In reply to youenn fablet from comment #7)
> Comment on attachment 324297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324297&action=review
> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEvent.cpp:37
> > +void PaymentRequestUpdateEvent::updateWith(RefPtr<DOMPromise>&&)
> 
> Ref<>?

Done.

> 
> > Source/WebCore/dom/PromiseRejectionEvent.h:38
> > +        RefPtr<DOMPromise> promise;
> 
> It should probably be a Ref<DOMPromise> as promise is required.
> 

I don't think this is currently possible because you can't default initialize it.

> > Source/WebCore/workers/service/FetchEvent.cpp:138
> > +        [] (Ref<FormData>&) {
> 
> Indentation like for regular switch?

Yeah. Trying to keep it consistent.

[youenn fablet]

> > 
> > > Source/WebCore/dom/PromiseRejectionEvent.h:38
> > > +        RefPtr<DOMPromise> promise;
> > 
> > It should probably be a Ref<DOMPromise> as promise is required.
> > 
> 
> I don't think this is currently possible because you can't default
> initialize it.

Should we try to handle dictionaries creation with std::optional<>?

[Sam Weinig]

(In reply to youenn fablet from comment #9)
> > > 
> > > > Source/WebCore/dom/PromiseRejectionEvent.h:38
> > > > +        RefPtr<DOMPromise> promise;
> > > 
> > > It should probably be a Ref<DOMPromise> as promise is required.
> > > 
> > 
> > I don't think this is currently possible because you can't default
> > initialize it.
> 
> Should we try to handle dictionaries creation with std::optional<>?

That would probably be a good way to handle it.

[Sam Weinig]

Committed r223725: <https://trac.webkit.org/changeset/223725>

[Radar WebKit Bug Importer]

<rdar://problem/35568654>