820957 - Support object member in WebIDL dictionary.

Status: RESOLVED FIXED

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

Description

[William Chen [:wchen]]

Currently an object member in a dictionary like this:

dictionary ElementRegistrationOptions {
  object? prototype = null;
};

Will output this during codegen:

"Can't handle member 'object'; need to sort out rooting issues"

Comment 1

[William Chen [:wchen]]

Attachment: Support object members in WebIDL dictionary.

Here is a patch that works for what I need by storing the object as a RootedObject in the dictionary struct. Feedback on how to implement this in a nice way would be appreciated.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691447 [details] [diff] [review]
Support object members in WebIDL dictionary.

You should really use the "feedback" flag...  ;)

The callback changes don't look right to me.  There's actually no rooting problem for non-worker callbacks now, so perhaps just change the check to isMember and descriptorProvider.workers?  That should be enough for your purposes.

You probably want to update the error message in the "object" case to mention "member not in a dictionary" or something.

>+            template = wrapObjectTemplate("${declName}.construct(cx, &${val}.toObject());",
>+                                          type, "", failureCode)

The "" looks wrong.  What happens if the type in the dictionary is "object?" and a null is passed for that dictionary member?

Note that I believe it's OK to construct a RootedObject with null, so that's what you likely want here.

That said, it seems like non-nullable objects in dictionaries should continue to look like JSObject& to consumers.  Might need another helper struct for that.

Apart from that and the spurious code movement at the bottom, this looks decent.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, one more caveat.  cx can be null if ${val} is JSVAL_NULL.  So you may need to deal with that.  It's worth looking at the resulting generated code to make sure that all works...

Comment 4

[William Chen [:wchen]]

Attachment: Support object members in WebIDL dictionary.

This patch fixes object members and callback function members. Let me know if you want me to split out the callback member into another bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692080 [details] [diff] [review]
Support object members in WebIDL dictionary.

>+++ b/dom/bindings/Codegen.py

>     if type.isCallback():
>-        if isMember:
>-            raise TypeError("Can't handle member callbacks; need to sort out "
>-                            "rooting issues")

That should probably just move into the desciptorProvider.workers clause, not go away completely.  And it should ideally throw a NoSuchDescriptorError (for why, see below).

>+            templateBody = "${declName}.construct(cx, &${val}.toObject());"
>+            templateBody = ("if (cx) {\n" +
>+                            CGIndenter(CGGeneric(templateBody)).define() +
>+                            "\n}")

I think this would be more readable as:

            templateBody = ("if (cx) {\n"
                            "  ${declName}.construct(cx, &${val}.toObject());\n"
                            "}")

and similar for setToNullCode.

>+            setToNullCode = "${declName}.construct(cx, (JSObject*) NULL);"

nullptr, please.  And do you actually need the cast?  I guess you do because it's a template?  If so, please document that.

>+            template = wrapObjectTemplate(templateBody, type, setToNullCode,
>+                                          failureCode)

Maybe put this outside the big if/else and just set templateBody and setToNullCode in the non-dictionary-member branch?

>@@ -6351,20 +6376,23 @@ class CGDictionary(CGThing):
>+                # Callback members with workers is currently not supported.
>+                # XXX: Sort out rooting issues.
>+                if not (member.isCallback() and desscriptorProvider.workers)]

"descriptor", one 's'.  Surprised this worked....

I think you don't need this if trying to wrap callbacks in workers throws NoSuchDescriptorError.

>+            }, True)

Please include the "argname=" before True, so it's clear what's being set to true.

r=me with those nits picked.  Thank you!

Comment 6

[William Chen [:wchen]]

Attachment: Support object members in WebIDL dictionary.

Comment 7

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f774c6094808

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f774c6094808