Bug 175246

Summary: [WebIDL] Add support for Promise<> attributes
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, darin, rniwa, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch
none
Patch
none
Patch ysuzuki: review+

Description Sam Weinig 2017-08-06 10:32:33 PDT 
[WebIDL] Add support for Promise<> attributes
Comment 1 Sam Weinig 2017-08-06 10:34:59 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-08-06 10:36:30 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2017-08-06 10:38:49 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-08-06 12:22:30 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-08-06 12:22:31 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-08-06 12:25:37 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-08-06 12:25:38 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-08-06 12:30:35 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-08-06 12:30:36 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-08-06 12:53:12 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-08-06 12:53:14 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-08-06 14:47:16 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-08-06 14:47:18 PDT Comment hidden (obsolete)
Comment 14 Sam Weinig 2017-08-06 15:55:12 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-08-06 15:57:30 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2017-08-06 18:43:32 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-08-06 18:45:36 PDT Comment hidden (obsolete)
Comment 18 Sam Weinig 2017-08-06 18:47:26 PDT 
Created attachment 317392 [details]
Patch
Comment 19 Build Bot 2017-08-06 18:48:32 PDT 
Attachment 317392 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/DOMPromiseProxy.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Sam Weinig 2017-08-06 19:01:06 PDT 
One thing to note, I'm not super thrilled with adding yet another promise related type. But I think I should be able merge something, probably DeferredPromise, into this one once I have converted more over to it.

Also, I feel DOMPromiseProxy is a pretty good name, despite using the horrid anti-word 'proxy', as it is a proxy for the real JS promise objects. But, I am open to other names.
Comment 21 Yusuke Suzuki 2017-08-08 07:31:14 PDT 
Comment on attachment 317392 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:70
> +            cycles when the resolved value is the owner such as is the case with FontFace and FontFaceSet.

OK, nice. I think noting this in DOMPromiseProxyWithResolveCallback code is nice.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:42
> +    using ValueOrException = Variant<Value, Exception>;

How about using WTF::Expected<Value, Exception> instead?

> Source/WebCore/bindings/js/DOMPromiseProxy.h:66
> +    using ValueOrException = Variant<Value, Exception>;

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:89
> +    using ValueOrException = Variant<Value, Exception>;

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:129
> +    // DeferredPromise can fail construction during worker abrupt termination.
> +    auto deferredPromise = DeferredPromise::create(state, globalObject, DeferredPromise::Mode::RetainPromiseOnResolve);

Great! I think we need to review the other DeferredPromise construction (JSPromiseDeferred::create use) as well (in a separate patch).

> Source/WebCore/bindings/js/DOMPromiseProxy.h:146
> +inline void DOMPromiseProxy<IDLType>::reset()

If it does not take any argument, I think the name `clear` is better for that.
When using `reset`, I expect that `reset` method takes an argument to be set.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:215
> +inline void DOMPromiseProxy<IDLVoid>::reset()

Ditto.

> Source/WebCore/bindings/js/DOMPromiseProxy.h:283
> +inline void DOMPromiseProxyWithResolveCallback<IDLType>::reset()

Ditto.
Comment 22 Sam Weinig 2017-08-08 18:01:46 PDT 
Committed r220433: <http://trac.webkit.org/changeset/220433>
Comment 23 Radar WebKit Bug Importer 2017-08-08 18:02:38 PDT 
<rdar://problem/33789891>
Comment 24 Sam Weinig 2017-08-08 20:24:44 PDT 
Additional changes landed in r220440.