Bug 178533

Summary: [Bindings] Standardize on DOMPromise as the way to store passed in promises
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, dbates, esprehn+autocc, kangil.han, kondapallykalyan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch youennf: review+

Description Sam Weinig 2017-10-19 12:29:47 PDT
[Bindings] Standarize on DOMPromise as the way to store passed in promises
Comment 1 Sam Weinig 2017-10-19 13:20:51 PDT
Created attachment 324270 [details]
Patch
Comment 2 youenn fablet 2017-10-19 14:12:21 PDT
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.
Comment 3 youenn fablet 2017-10-19 14:14:46 PDT
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.
Comment 4 Sam Weinig 2017-10-19 14:22:46 PDT
(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.
Comment 5 Sam Weinig 2017-10-19 14:29:33 PDT
(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.
Comment 6 Sam Weinig 2017-10-19 14:51:18 PDT
Created attachment 324297 [details]
Patch
Comment 7 youenn fablet 2017-10-19 15:44:46 PDT
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?
Comment 8 Sam Weinig 2017-10-19 15:50:23 PDT
(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.
Comment 9 youenn fablet 2017-10-19 15:54:02 PDT
> > 
> > > 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<>?
Comment 10 Sam Weinig 2017-10-19 16:29:01 PDT
(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.
Comment 11 Sam Weinig 2017-10-19 16:31:05 PDT
Committed r223725: <https://trac.webkit.org/changeset/223725>
Comment 12 Radar WebKit Bug Importer 2017-11-15 13:02:10 PST
<rdar://problem/35568654>