807224 - Add native-to-JS conversion for unions

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement native-to-JS conversion for WebIDL unions.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With cleaner handling for object.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Now actually compiling and all without landing us in include hell all over the codebase

Comment 4

[Peter Van der Beken [: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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2777bc29ce77 addresses the review comments.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99665db01039
https://hg.mozilla.org/mozilla-central/rev/2777bc29ce77