Bug 175246 - [WebIDL] Add support for Promise<> attributes
Summary: [WebIDL] Add support for Promise<> attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-06 10:32 PDT by Sam Weinig
Modified: 2017-08-08 20:24 PDT (History)
6 users (show)

See Also:

Attachments
Patch (99.57 KB, patch)
2017-08-06 10:34 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.95 MB, application/zip)
2017-08-06 12:22 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.81 MB, application/zip)
2017-08-06 12:25 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.00 MB, application/zip)
2017-08-06 12:30 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.78 MB, application/zip)
2017-08-06 12:53 PDT, Build Bot 		no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (2.78 MB, application/zip)
2017-08-06 14:47 PDT, Build Bot 		no flags Details
Patch (112.27 KB, patch)
2017-08-06 15:55 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (115.73 KB, patch)
2017-08-06 18:43 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (115.73 KB, patch)
2017-08-06 18:47 PDT, Sam Weinig 		ysuzuki: review+
Details | Formatted Diff | Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.