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...
Created attachment 288784 [details] Patch
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.
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
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
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
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
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
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
Created attachment 288808 [details] Patch
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.
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
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
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
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
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
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
Created attachment 288816 [details] Patch
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.
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?
(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?
(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?
Created attachment 289058 [details] Patch
Created attachment 289061 [details] Patch
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.
Created attachment 289063 [details] Patch
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.
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.
(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.
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.
(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
Created attachment 289439 [details] Patch
(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.
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.
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
Created attachment 289526 [details] Patch for landing
Comment on attachment 289526 [details] Patch for landing Clearing flags on attachment: 289526 Committed r206252: <http://trac.webkit.org/changeset/206252>
All reviewed patches have been landed. Closing bug.