Closed Bug 756256 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
I used NonNull<JSObject> and a (JSObject&) cast. Let me know if you prefer me to use JSObject*.
Attachment #624882 - Flags: review?(bzbarsky)
Attachment #624882 - Attachment is obsolete: true
Attachment #624882 - Flags: review?(bzbarsky)
Attached patch v1 (obsolete) — Splinter Review
Attachment #624902 - Flags: review?(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 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.
Attachment #624902 - Flags: review?(bzbarsky) → review-
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?
(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.
Attached patch v2Splinter Review
Attachment #624902 - Attachment is obsolete: true
Attachment #625267 - Flags: review?(bzbarsky)
Comment on attachment 625267 [details] [diff] [review]
v2

r=me.  This is lovely!
Attachment #625267 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/8a8c60825d86
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 773326
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.