Last Comment Bug 756256 - Add support for object arguments and return types in new DOM bindings
: Add support for object arguments and return types in new DOM bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 773326
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-17 13:59 PDT by Peter Van der Beken [:peterv]
Modified: 2012-07-12 09:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (10.61 KB, patch)
2012-05-17 13:59 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1 (10.72 KB, patch)
2012-05-17 14:31 PDT, Peter Van der Beken [:peterv]
bzbarsky: review-
Details | Diff | Splinter Review
v2 (17.78 KB, patch)
2012-05-18 14:32 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2012-05-17 13:59:48 PDT
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*.
Comment 1 Peter Van der Beken [:peterv] 2012-05-17 14:31:53 PDT
Created attachment 624902 [details] [diff] [review]
v1
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-05-17 17:02:23 PDT
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 [:bz] (still a bit busy) 2012-05-17 22:45:38 PDT
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 (⌚ UTC+1/+2) 2012-05-18 00:46:08 PDT
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] 2012-05-18 07:45:22 PDT
(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] 2012-05-18 14:32:53 PDT
Created attachment 625267 [details] [diff] [review]
v2
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-05-21 14:40:43 PDT
Comment on attachment 625267 [details] [diff] [review]
v2

r=me.  This is lovely!
Comment 8 Peter Van der Beken [:peterv] 2012-05-22 14:32:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c60825d86
Comment 9 Ed Morley [:emorley] 2012-05-23 05:19:32 PDT
https://hg.mozilla.org/mozilla-central/rev/8a8c60825d86

Note You need to log in before you can comment on or make changes to this bug.