WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Patch
(67.98 KB, patch)
2016-09-14 05:35 PDT
,
youenn fablet
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Patch
(68.05 KB, patch)
2016-09-14 08:25 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(78.49 KB, patch)
2016-09-16 05:58 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(78.12 KB, patch)
2016-09-16 07:35 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(78.33 KB, patch)
2016-09-16 08:07 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(78.25 KB, patch)
2016-09-21 02:39 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(79.50 KB, patch)
2016-09-22 00:05 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-09-14 01:23:50 PDT
Created
attachment 288784
[details]
Patch
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
Created
attachment 288808
[details]
Patch
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
Created
attachment 288816
[details]
Patch
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
Created
attachment 289058
[details]
Patch
youenn fablet
Comment 23
2016-09-16 07:35:49 PDT
Created
attachment 289061
[details]
Patch
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
Created
attachment 289063
[details]
Patch
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
Created
attachment 289439
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug