756256 - Add support for object arguments and return types in new DOM bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

I used NonNull<JSObject> and a (JSObject&) cast. Let me know if you prefer me to use JSObject*.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 624902 [details] [diff] [review]
v1

I assume that simply ditching NonNull (which I invented so I wouldn't need to add casts at the call point because I'd thought it would be a lot harder than this) and using the cast plus a derefernce would involve too much complicated code in CGCallGenerator.__init__ ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 624902 [details] [diff] [review]
v1

>+++ b/dom/bindings/Codegen.py
>@@ -1472,16 +1472,38 @@ for (uint32_t i = 0; i < length; ++i) {
>+                            "  return Throw<false>(cx, NS_ERROR_XPC_BAD_CONVERT_JS);\n"

Should this not involve |failureCode| somehow?  Seems like the code as written would break if a method that overloads "object" and "long" were passed a number.  And also, pass the right bool to Throw()?

This applies in both places where you throw in this code.

Chances are we want to return True, not False, for callback in getRetvalDeclarationForType.  But separate bug is fine for that.

>+            # This is a workaround for a bug in Apple's clang.

"On 10.7", perhaps?

>+            if a.type.isObject():
>+                arg = "(JSObject&)" + arg

You need to check for non-nullable.  We really need to get that test infrastructure patch landed.  ;)

>+        if needsCx or 'implicitJSContext' in extendedAttributes:
>+            args.prepend(CGGeneric("cx"))

This changes the signature so that "cx" now comes after argspre, whereas before it was the first arg.  I'm surprised this didn't break any consumers.  Presumably we don't have any constructors taking "object" yet?  It looks like if we did, this would put the cx after the global (which is not desirable).  It also looks like it will pass cx twice for such a constructor in workers, which is also suboptimal.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 624902 [details] [diff] [review]
v1

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

::: dom/bindings/Codegen.py
@@ +1482,5 @@
> +            raise TypeError("Can't handle sequences of 'object'")
> +        if type.nullable():
> +            if not isDefinitelyObject:
> +                template = ("if (${val}.isObjectOrNull()) {\n"
> +                            "  ${declName} = &${val}.toObjectOrNull();\n"

Isn't &${val}.toObjectOrNull() a JSObject**?

@@ +1854,5 @@
>          # XXXbz we're going to assume that callback types are always
>          # nullable for now.
> +        return CGGeneric("JSObject*"), False
> +    if returnType.tag() is IDLType.Tags.any:
> +        return CGGeneric("JS::Value"), False

True?

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Should this not involve |failureCode| somehow?  Seems like the code as
> written would break if a method that overloads "object" and "long" were
> passed a number.  And also, pass the right bool to Throw()?

I started using wrapObjectTemplate for this.

> Chances are we want to return True, not False, for callback in
> getRetvalDeclarationForType.  But separate bug is fine for that.

Yeah, I decided to not change this for existing types.

> You need to check for non-nullable.  We really need to get that test
> infrastructure patch landed.  ;)

Yup.

> This changes the signature so that "cx" now comes after argspre, whereas
> before it was the first arg.  I'm surprised this didn't break any consumers.
> Presumably we don't have any constructors taking "object" yet?  It looks
> like if we did, this would put the cx after the global (which is not
> desirable).  It also looks like it will pass cx twice for such a constructor
> in workers, which is also suboptimal.

argsPre is a pain. There's no way to know what arguments have been passed in it, there could easily be conflicts (as is the case here). I'll see if I can fix this for cx.

(In reply to Ms2ger from comment #4)
> Isn't &${val}.toObjectOrNull() a JSObject**?

This is gone now.

> True?

See above.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 625267 [details] [diff] [review]
v2

r=me.  This is lovely!

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c60825d86

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a8c60825d86