Bug 161787 – Improve DeferredWrapper code

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

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...

[youenn fablet]

Created attachment 288784 [details]
Patch

[WebKit Commit Bot]

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 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]

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 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]

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 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]

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]

Created attachment 288808 [details]
Patch

[WebKit Commit Bot]

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 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]

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 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]

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 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]

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]

Created attachment 288816 [details]
Patch

[WebKit Commit Bot]

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 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]

(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]

(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]

Created attachment 289058 [details]
Patch

[youenn fablet]

Created attachment 289061 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 289063 [details]
Patch

[WebKit Commit Bot]

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 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]

(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 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]

(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]

Created attachment 289439 [details]
Patch

[youenn fablet]

(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]

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 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]

Created attachment 289526 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289526 [details]
Patch for landing

Clearing flags on attachment: 289526

Committed r206252: <http://trac.webkit.org/changeset/206252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.