Closed Bug 1455217 Opened 4 years ago Closed 4 years ago

Add an explicit promise type to xpidl


(Core :: XPCOM, enhancement)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: nika, Assigned: nika)




(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

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

Attachment #8972166 - Flags: review?(bzbarsky) → review+
Pushed by
Part 1: Add an explicit Promise type to xpidl, r=mccr8
Part 2: Add support for promises to XPCConvert, r=mccr8
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.