Closed Bug 820957 Opened 9 years ago Closed 9 years ago

Support object member in WebIDL dictionary.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: wchen, Assigned: wchen)

References

Details

Attachments

(1 file, 2 obsolete files)

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"
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 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.
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...
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.
Attachment #691447 - Attachment is obsolete: true
Attachment #692080 - Flags: review?(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!
Attachment #692080 - Flags: review?(bzbarsky) → review+
Attachment #692080 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/f774c6094808
Assignee: nobody → wchen
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f774c6094808
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.