RESOLVED FIXED 161787
Improve DeferredWrapper code
https://bugs.webkit.org/show_bug.cgi?id=161787
Summary Improve DeferredWrapper code
youenn fablet
Reported 2016-09-09 01:52:51 PDT
We should use more references for DeferredWrapper. We might also want to make JSPromiseDeferred creation within DeferredWrapper. We might want to rename DeferredWrapper to a more meaningful name like PromiseDeferred, PromiseDeferredWrapper, GenericDOMPromise...
Attachments
Patch (67.98 KB, patch)
2016-09-14 01:23 PDT, youenn fablet
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite (899.53 KB, application/zip)
2016-09-14 02:25 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.12 MB, application/zip)
2016-09-14 02:29 PDT, Build Bot
no flags
Archive of layout-test-results from ews117 for mac-yosemite (1.55 MB, application/zip)
2016-09-14 02:32 PDT, Build Bot
no flags
Patch (67.98 KB, patch)
2016-09-14 05:35 PDT, youenn fablet
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite (1.04 MB, application/zip)
2016-09-14 06:38 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (957.25 KB, application/zip)
2016-09-14 06:40 PDT, Build Bot
no flags
Archive of layout-test-results from ews117 for mac-yosemite (1.52 MB, application/zip)
2016-09-14 06:43 PDT, Build Bot
no flags
Patch (68.05 KB, patch)
2016-09-14 08:25 PDT, youenn fablet
no flags
Patch (78.49 KB, patch)
2016-09-16 05:58 PDT, youenn fablet
no flags
Patch (78.12 KB, patch)
2016-09-16 07:35 PDT, youenn fablet
no flags
Patch (78.33 KB, patch)
2016-09-16 08:07 PDT, youenn fablet
no flags
Patch (78.25 KB, patch)
2016-09-21 02:39 PDT, youenn fablet
no flags
Patch for landing (79.50 KB, patch)
2016-09-22 00:05 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2016-09-14 01:23:50 PDT
WebKit Commit Bot
Comment 2 2016-09-14 01:25:23 PDT
Attachment 288784 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 3 2016-09-14 02:24:59 PDT
Comment on attachment 288784 [details] Patch Attachment 288784 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2070917 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 4 2016-09-14 02:25:01 PDT
Created attachment 288790 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 5 2016-09-14 02:29:07 PDT
Comment on attachment 288784 [details] Patch Attachment 288784 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2070921 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 6 2016-09-14 02:29:09 PDT
Created attachment 288791 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 7 2016-09-14 02:32:43 PDT
Comment on attachment 288784 [details] Patch Attachment 288784 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2070920 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 8 2016-09-14 02:32:45 PDT
Created attachment 288792 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
youenn fablet
Comment 9 2016-09-14 05:35:09 PDT
WebKit Commit Bot
Comment 10 2016-09-14 05:36:14 PDT
Attachment 288808 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 11 2016-09-14 06:38:18 PDT
Comment on attachment 288808 [details] Patch Attachment 288808 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2072014 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 12 2016-09-14 06:38:21 PDT
Created attachment 288809 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 13 2016-09-14 06:40:07 PDT
Comment on attachment 288808 [details] Patch Attachment 288808 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2072030 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 14 2016-09-14 06:40:10 PDT
Created attachment 288810 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 15 2016-09-14 06:43:43 PDT
Comment on attachment 288808 [details] Patch Attachment 288808 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2072029 New failing tests: fast/custom-elements/CustomElementRegistry.html
Build Bot
Comment 16 2016-09-14 06:43:45 PDT
Created attachment 288811 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
youenn fablet
Comment 17 2016-09-14 08:25:49 PDT
WebKit Commit Bot
Comment 18 2016-09-14 08:27:45 PDT
Attachment 288816 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Sam Weinig
Comment 19 2016-09-15 06:30:02 PDT
Comment on attachment 288816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288816&action=review > Source/WebCore/bindings/js/JSDOMPromise.h:107 > + static Ref<DOMDeferredPromise> create(JSC::ExecState& state, JSDOMGlobalObject& globalObject) > { > - return adoptRef(*new DeferredWrapper(state, globalObject, deferred)); > + JSC::JSPromiseDeferred* deferred = JSC::JSPromiseDeferred::create(&state, &globalObject); > + // deferred can only be null in workers. > + ASSERT(deferred); > + return adoptRef(*new DOMDeferredPromise(globalObject, *deferred)); > } If this is only for use in non-worker environments, should this take a JSDOMWindow instead?
youenn fablet
Comment 20 2016-09-15 06:38:36 PDT
(In reply to comment #19) > Comment on attachment 288816 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288816&action=review > > > Source/WebCore/bindings/js/JSDOMPromise.h:107 > > + static Ref<DOMDeferredPromise> create(JSC::ExecState& state, JSDOMGlobalObject& globalObject) > > { > > - return adoptRef(*new DeferredWrapper(state, globalObject, deferred)); > > + JSC::JSPromiseDeferred* deferred = JSC::JSPromiseDeferred::create(&state, &globalObject); > > + // deferred can only be null in workers. > > + ASSERT(deferred); > > + return adoptRef(*new DOMDeferredPromise(globalObject, *deferred)); > > } > > If this is only for use in non-worker environments, should this take a > JSDOMWindow instead? Right, but we usually manipulate JSDOMGlobalObject in the binding code so that would need a cast in the caller side. We could add a domWindow() getter in JSDOMObject to ease that. How about just adding an ASSERT to check this is a JSDOMWindow in the DOMDeferredPromise::create method?
Sam Weinig
Comment 21 2016-09-15 12:26:09 PDT
(In reply to comment #20) > (In reply to comment #19) > > Comment on attachment 288816 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=288816&action=review > > > > > Source/WebCore/bindings/js/JSDOMPromise.h:107 > > > + static Ref<DOMDeferredPromise> create(JSC::ExecState& state, JSDOMGlobalObject& globalObject) > > > { > > > - return adoptRef(*new DeferredWrapper(state, globalObject, deferred)); > > > + JSC::JSPromiseDeferred* deferred = JSC::JSPromiseDeferred::create(&state, &globalObject); > > > + // deferred can only be null in workers. > > > + ASSERT(deferred); > > > + return adoptRef(*new DOMDeferredPromise(globalObject, *deferred)); > > > } > > > > If this is only for use in non-worker environments, should this take a > > JSDOMWindow instead? > > Right, but we usually manipulate JSDOMGlobalObject in the binding code so > that would need a cast in the caller side. We could add a domWindow() getter > in JSDOMObject to ease that. > > How about just adding an ASSERT to check this is a JSDOMWindow in the > DOMDeferredPromise::create method? I think using the correct type, and making it the callers job makes more sense. Otherwise, it seems to easy to get wrong. Also, would the name DOMPromiseDeferred make more sense to map to JSPromiseDeferred?
youenn fablet
Comment 22 2016-09-16 05:58:50 PDT
youenn fablet
Comment 23 2016-09-16 07:35:49 PDT
WebKit Commit Bot
Comment 24 2016-09-16 07:36:50 PDT
Attachment 289061 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:204: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:206: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
youenn fablet
Comment 25 2016-09-16 08:07:46 PDT
WebKit Commit Bot
Comment 26 2016-09-16 08:09:56 PDT
Attachment 289063 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:204: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:206: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 27 2016-09-19 13:46:05 PDT
Comment on attachment 289063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289063&action=review > Source/WebCore/ChangeLog:10 > + Renaming DeferredWrapper to DOMPromiseDeferred. We normally don’t like to use DOM in the names of things unless we are truly forced to. DOMWindow is a necessary evil since the term "window" doesn’t make much sense without the DOM prefix, but even that one should probably be named something else that‘s more about its role as the global object for a webpage. I’m not sure I like the direction of this renaming. Do we have some other kind of promise that we have to disambiguate from a "DOM promise"? My name for this would be DeferredPromise since it is a “deferred promise”. But apparently we have something named JSPromiseDeferred. That name doesn’t seem so great to me either.
youenn fablet
Comment 28 2016-09-20 00:59:24 PDT
(In reply to comment #27) > Comment on attachment 289063 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289063&action=review > > > Source/WebCore/ChangeLog:10 > > + Renaming DeferredWrapper to DOMPromiseDeferred. > > We normally don’t like to use DOM in the names of things unless we are truly > forced to. DOMWindow is a necessary evil since the term "window" doesn’t > make much sense without the DOM prefix, but even that one should probably be > named something else that‘s more about its role as the global object for a > webpage. We could rename DOMGenericPromise to GenericPromise or UntypedPromise. And DOMPromise to TypedPromise or simply Promise. I think the term is now coined to this feature. > I’m not sure I like the direction of this renaming. Do we have some other > kind of promise that we have to disambiguate from a "DOM promise"? My name > for this would be DeferredPromise since it is a “deferred promise”. But > apparently we have something named JSPromiseDeferred. That name doesn’t seem > so great to me either. (DOM)DeferredPromise sounds better to me too. I reverted to (DOM)PromiseDeferred in the latest patch for consistency with JSPromiseDeferred. I am not sure why JSPromiseDeferred was named this way. Probably we could/should switch to DeferredPromise.
Ryosuke Niwa
Comment 29 2016-09-20 02:01:45 PDT
Comment on attachment 289063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289063&action=review > Source/WebCore/ChangeLog:11 > + Introduced DOMGenericPromise as a ref of DOMPromiseDeferred to improve readability of code. I don't think this is a good idea. Aliasing a fundamental template type such as Ref to something else makes it rather hard to follow the code.
youenn fablet
Comment 30 2016-09-21 00:27:36 PDT
(In reply to comment #29) > Comment on attachment 289063 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289063&action=review > > > Source/WebCore/ChangeLog:11 > > + Introduced DOMGenericPromise as a ref of DOMPromiseDeferred to improve readability of code. > > I don't think this is a good idea. > Aliasing a fundamental template type such as Ref to something else makes it > rather hard to follow the code. I think it improves the code in fetch module for instance. This allows having the same code style for untyped promises vs. typed promises. I agree it is not that good to use in JSDOMPromise.h though. I'll prepare a patch with these two changes: - Renaming DOMPromiseDeferred to DeferredPromise - Making use of GenericPromise only in fetch code
youenn fablet
Comment 31 2016-09-21 02:39:41 PDT
youenn fablet
Comment 32 2016-09-21 02:41:46 PDT
(In reply to comment #31) > Created attachment 289439 [details] > Patch I removed GenericPromise from now but renamed DOMPromiseDeferred to DeferredPromise. If there is no objection, I'll land the patch tomorrow.
WebKit Commit Bot
Comment 33 2016-09-21 02:42:55 PDT
Attachment 289439 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:203: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:205: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 3 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 34 2016-09-21 23:37:51 PDT
Comment on attachment 289439 [details] Patch Rejecting attachment 289439 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 289439, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ource/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp patching file Source/WebCore/bindings/scripts/CodeGeneratorJS.pm patching file Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp patching file Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp patching file Source/WebCore/dom/CustomElementRegistry.cpp patching file Source/WebCore/dom/CustomElementRegistry.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/2123544
youenn fablet
Comment 35 2016-09-22 00:05:00 PDT
Created attachment 289526 [details] Patch for landing
WebKit Commit Bot
Comment 36 2016-09-22 00:38:01 PDT
Comment on attachment 289526 [details] Patch for landing Clearing flags on attachment: 289526 Committed r206252: <http://trac.webkit.org/changeset/206252>
WebKit Commit Bot
Comment 37 2016-09-22 00:38:08 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.