Add native-to-JS conversion for unions

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 677551 [details] [diff] [review]
Implement native-to-JS conversion for WebIDL unions.
Attachment #677551 - Flags: review?(peterv)
Created attachment 677560 [details] [diff] [review]
With cleaner handling for object.
Attachment #677560 - Flags: review?(peterv)
Attachment #677551 - Attachment is obsolete: true
Attachment #677551 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Created attachment 677629 [details] [diff] [review]
Now actually compiling and all without landing us in include hell all over the codebase
Attachment #677629 - Flags: review?(peterv)
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
You need to log in before you can comment on or make changes to this bug.