RESOLVED FIXED175246
[WebIDL] Add support for Promise<> attributes 
https://bugs.webkit.org/show_bug.cgi?id=175246
Summary [WebIDL] Add support for Promise<> attributes
Sam Weinig
Reported 2017-08-06 10:32:33 PDT
[WebIDL] Add support for Promise<> attributes
Attachments
Patch (99.57 KB, patch)
2017-08-06 10:34 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (115.73 KB, patch)
2017-08-06 18:43 PDT, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (115.73 KB, patch)
2017-08-06 18:47 PDT, Sam Weinig 		ysuzuki: review+
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Sam Weinig
Comment 1 2017-08-06 10:34:59 PDT Comment hidden (obsolete)
Build Bot
Comment 2 2017-08-06 10:36:30 PDT Comment hidden (obsolete)
Build Bot
Comment 3 2017-08-06 10:38:49 PDT Comment hidden (obsolete)
Build Bot
Comment 4 2017-08-06 12:22:30 PDT Comment hidden (obsolete)
Build Bot
Comment 5 2017-08-06 12:22:31 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2017-08-06 12:25:37 PDT Comment hidden (obsolete)
Build Bot
Comment 7 2017-08-06 12:25:38 PDT Comment hidden (obsolete)
Build Bot
Comment 8 2017-08-06 12:30:35 PDT Comment hidden (obsolete)
Build Bot
Comment 9 2017-08-06 12:30:36 PDT Comment hidden (obsolete)
Build Bot
Comment 10 2017-08-06 12:53:12 PDT Comment hidden (obsolete)
Build Bot
Comment 11 2017-08-06 12:53:14 PDT Comment hidden (obsolete)
Build Bot
Comment 12 2017-08-06 14:47:16 PDT Comment hidden (obsolete)
Build Bot
Comment 13 2017-08-06 14:47:18 PDT Comment hidden (obsolete)
Sam Weinig
Comment 14 2017-08-06 15:55:12 PDT Comment hidden (obsolete)
Build Bot
Comment 15 2017-08-06 15:57:30 PDT Comment hidden (obsolete)
Sam Weinig
Comment 16 2017-08-06 18:43:32 PDT Comment hidden (obsolete)
Build Bot
Comment 17 2017-08-06 18:45:36 PDT Comment hidden (obsolete)
Sam Weinig
Comment 18 2017-08-06 18:47:26 PDT
Created attachment 317392 [details] Patch
Build Bot
Comment 19 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.
Sam Weinig
Comment 20 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.
Yusuke Suzuki
Comment 21 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.
Sam Weinig
Comment 22 2017-08-08 18:01:46 PDT
Committed r220433: <http://trac.webkit.org/changeset/220433>
Radar WebKit Bug Importer
Comment 23 2017-08-08 18:02:38 PDT
<rdar://problem/33789891>
Sam Weinig
Comment 24 2017-08-08 20:24:44 PDT
Additional changes landed in r220440.
Note You need to log in before you can comment on or make changes to this bug.