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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 624882 [details] [diff] [review]
v1

I used NonNull<JSObject> and a (JSObject&) cast. Let me know if you prefer me to use JSObject*.
Attachment #624882 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #624882 - Attachment is obsolete: true
Attachment #624882 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

5 years ago
Created attachment 624902 [details] [diff] [review]
v1
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?
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 625267 [details] [diff] [review]
v2
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+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c60825d86
Target Milestone: --- → mozilla15

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/8a8c60825d86
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 773326
You need to log in before you can comment on or make changes to this bug.