Closed Bug 807224 Opened 7 years ago Closed 7 years ago

Add native-to-JS conversion for unions

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I'll need this for eventhandler stuff. Note that this might not be the same as supporting union return values, because pure JS-to-native conversion (e.g. for arguments to a callback that the caller of the callback is holding strong refs to) may be possible without worrying about ownership issues, whereas for return values we need to worry about ownership and lifetimes.
Blocks: 807226
Attachment #677560 - Flags: review?(peterv)
Attachment #677551 - Attachment is obsolete: true
Attachment #677551 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #677560 - Attachment is obsolete: true
Attachment #677560 - Flags: review?(peterv)
Comment on attachment 677629 [details] [diff] [review]
Now actually compiling and all without landing us in include hell all over the codebase

Review of attachment 677629 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +4477,5 @@
> +        conversionsToJS = []
> +        if self.type.hasNullableType:
> +            conversionsToJS.append("    case eNull:\n"
> +                                   "      *vp = JS::NullValue();\n"
> +                                   "      break;")

You put braces around the other cases, so here too please.
Seems to me like this should return true instead of the break.

@@ +4481,5 @@
> +                                   "      break;")
> +        conversionsToJS.extend(
> +            caseDecl + caseBody for (caseDecl, caseBody) in
> +            zip(mapTemplate("    case e${name}:\n", templateVars),
> +                map(self.getConversiontoJS,

Could we add the case statement in getConversiontoJS instead of this zip and iterate.

@@ +4491,5 @@
> +  switch (mType) {
> +${doConversionsToJS}
> +
> +    case eUninitialized:
> +      break;

Braces.

@@ +4505,5 @@
> +        (templateVars, type) = arg
> +        val = "mValue.m%(name)s.Value()" % templateVars
> +        if type.isString():
> +            # XPConnect string-to-JS conversion wants to mutate the string.  So
> +            # let's give it a string it can mutate

Where does that happen?

@@ +4511,5 @@
> +            prepend = "nsString mutableStr(%s);\n" % val
> +            val = "mutableStr"
> +        else:
> +            prepend = ""
> +            if type.isObject() and not type.nullable():

I don't think flatMemberTypes ever has a nullable type, so drop the .nullable check.
Attachment #677629 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/99665db01039

Except I suck and failed to notice that the review comments _just_ came in....  Will address in a followup push in a few mins.

> You put braces around the other cases, so here too please.
> Seems to me like this should return true instead of the break.

Agreed, done.

> Could we add the case statement in getConversiontoJS instead of this zip and iterate.

Yes!  It wasn't possible in an earlier patch iteration, but now it's totally doable.  Changing the return statement in getConversionToJS to:

        return CGIndenter(CGList([CGGeneric("case e%(name)s:" % templateVars),
                                  CGWrapper(CGIndenter(CGGeneric(wrapCode)),
                                            pre="{\n",
                                            post="\n}")],
                                 "\n"),
                          4).define()

does the trick.

> Braces.

Done.

> Where does that happen?

Which?  The XPConnect mutation?  When it attempts to steal the stringbuffer from the string to avoid refcounting it.

> I don't think flatMemberTypes ever has a nullable type, so drop the .nullable check.

OK, changed that to an assert.
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.