Closed Bug 1455217 Opened 3 years ago Closed 3 years ago

Add an explicit promise type to xpidl

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(3 files, 1 obsolete file)

Currently we pass promises as nsISupports through xpidl. It would be nicer if we could instead use an explicit 'promise' type in xpidl which would be typed as 'mozilla::dom::Promise' in C++.

This also opens the way for us potentially removing the custom logic in XPConnect for passing promises as nsISupports.
This type is fairly simple on the idl parsing side of things. I handle it in the
same way that special types such as ns[C]String, nsid, and jsval are handled, by
using a special native type.

The logic for converting the value between C++ and JS follows the existing logic
from the nsISupports <=> JS Promise conversions.
Attachment #8969168 - Flags: review?(continuation)
These are the places in our tree where I noticed we were passing Promise objects
over XPIDL as nsISupports. I didn't change places where we pass promises as
`jsval`.

It is very likely that I have missed some consumers of the old behaviour in this
quick pass.
Attachment #8969169 - Flags: review?(bzbarsky)
Comment on attachment 8969168 [details] [diff] [review]
Part 1: Add an explicit Promise type to xpidl

Review of attachment 8969168 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +190,5 @@
>    TD_CSTRING           = 24,
>    TD_ASTRING           = 25,
>    TD_JSVAL             = 26,
> +  TD_DOMOBJECT         = 27,
> +  TD_PROMISE           = 28

Maybe you should paste the warning at the top of the type in here (about there being only 32 tags allowed) so that it'll show up here when somebody is reviewing an addition?
Attachment #8969168 - Flags: review?(continuation) → review+
Comment on attachment 8969169 [details] [diff] [review]
Part 2: Use the new xpidl Promise type instead of nsISupports

>+++ b/js/xpconnect/src/XPCConvert.cpp
>+        if (IsPromiseObject(src)) {

You probably need a CheckedUnwrap somewhere in here.  Or alternately we could change semantics a bit and do Promise::Resolve, which will handle wrappers correctly.

>+            nsIGlobalObject* glob = NativeGlobal(src);

This is the same as NativeGlobal(CurrentGlobalOrNull(cx)), right?  Might be worth asserting that.

>+        if (cleanupMode == re) { 

Stray space got added.

>+            return *((Promise**)d);

Why not just "return true"?  CreateFromExisting never returns null.

r=me on the rest, but I'd like to see the updated xpcconvert code.
Attachment #8969169 - Flags: review?(bzbarsky) → review-
Attachment #8969169 - Attachment is obsolete: true
Comment on attachment 8972165 [details] [diff] [review]
Part 2: Add support for promises to XPCConvert

Review of attachment 8972165 [details] [diff] [review]:
-----------------------------------------------------------------

Boris is more likely than me to be able to provide a useful review on this code.
Attachment #8972165 - Flags: review?(continuation) → review?(bzbarsky)
Comment on attachment 8972165 [details] [diff] [review]
Part 2: Add support for promises to XPCConvert

>+        d.setObjectOrNull(jsobj);

Can jsobj be null here?  If not (and I suspect it can't), d.setObject(*jsobj) is likely clearer.

>+        if (!JS_WrapValue(cx, &src)) {

Why do you need that?  I'd expect "s" to be in the compartment of "cx" here (and certainly other parts of JSData2Native assume that).  I don't think you need the src temporary at all.

r=me modulo those bits.
Attachment #8972165 - Flags: review?(bzbarsky) → review+
Comment on attachment 8972166 [details] [diff] [review]
Part 3: Use the new xpidl Promise type instead of nsISupports

r=me
Attachment #8972166 - Flags: review?(bzbarsky) → review+
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e62b365b36
Part 1: Add an explicit Promise type to xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0ed3df62bd
Part 2: Add support for promises to XPCConvert, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2fa34896c4
Part 3: Use the new xpidl Promise type instead of nsISupports, r=bz
You need to log in before you can comment on or make changes to this bug.