1455217 - Add an explicit promise type to xpidl

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an explicit Promise type to xpidl

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.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Use the new xpidl Promise type instead of nsISupports

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.

Comment 3

[Andrew McCreight [:mccr8]]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add support for promises to XPCConvert

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Use the new xpidl Promise type instead of nsISupports

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8972166 [details] [diff] [review]
Part 3: Use the new xpidl Promise type instead of nsISupports

r=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/15e62b365b36
https://hg.mozilla.org/mozilla-central/rev/6b0ed3df62bd
https://hg.mozilla.org/mozilla-central/rev/5e2fa34896c4