Closed
Bug 756256
Opened 13 years ago
Closed 13 years ago
Add support for object arguments and return types in new DOM bindings
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: peterv, Assigned: peterv)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | 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)
Assignee | ||
Updated•13 years ago
|
Attachment #624882 -
Attachment is obsolete: true
Attachment #624882 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #624902 -
Flags: review?(bzbarsky)
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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 4•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #624902 -
Attachment is obsolete: true
Attachment #625267 -
Flags: review?(bzbarsky)
Comment 7•13 years ago
|
||
Comment on attachment 625267 [details] [diff] [review]
v2
r=me. This is lovely!
Attachment #625267 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•13 years ago
|
||
Target Milestone: --- → mozilla15
Comment 9•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•