865998 - Implement WebIDL union return values

Status: RESOLVED FIXED

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

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Generated code

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 742223 [details] [diff] [review]
Patch

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +1255,1 @@
>    WarnAboutUnexpectedStyle(mCanvasElement);

This can become MOZ_NOT_REACHED now

::: dom/bindings/Codegen.py
@@ +5579,5 @@
> +        self.descriptorProvider = descriptorProvider
> +        self.templateVars = map(
> +            lambda t: getUnionTypeTemplateVars(t, self.descriptorProvider,
> +                                               isReturnValue=True),
> +            self.type.flatMemberTypes)

self.templateVars = [getUnionTypeTemplateVars(t, self.descriptorProvider, isReturnValue=True) for t in self.type.flatMemberTypes]

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

I changed the generated destructor to destroy the members directly instead of calling the functions.  That way we don't need the Is__ and Destroy__ functions.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Did you mean to ask for review on that?  (Note that I won't get to it until Monday at best...)

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Would be nice to use them for nsGenericHTMLElement::GetItemValue too, though I guess we'd need the different-getter-and-setter-types bug too.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742429 [details] [diff] [review]
Updated patch

The attached generated code is not holding owning references to the interface pointers, right?  Shouldn't it be?

Comment 10

[David Zbarsky (:dzbarsky)]

It doesn't matter in this case because the context keeps them alive.  But yeah, I guess it should in general.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Right; the problem is that the union has no way to know...

I wonder if we should go ahead and implement the thing that will automagically hold a ref or not depending on whether you assign an nsIFoo* or an already_AddRefed<nsIFoo> to it and use it here.  Or just hold refs always for now.

Comment 12

[David Zbarsky (:dzbarsky)]

That would be nice.  Did smaug ever finish that patch?

Comment 13

[Boris Zbarsky [:bzbarsky]]

No, but I think we should do it....

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Patch

I switched to OwningNonNull for now.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 748502 [details] [diff] [review]
Patch

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

::: dom/bindings/Codegen.py
@@ +5775,5 @@
> +  }"""
> +        methods.extend(mapTemplate(methodTemplate, templateVars))
> +        values = mapTemplate("UnionMember<${structType} > m${name};", templateVars)
> +        return string.Template("""
> +class ${structName}ReturnValue {

This might look better using CGClass

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 762533 [details] [diff] [review]
Updated patch

>+++ b/content/base/src/moz.build
>+    'nsImageLoadingContent.h',

Why does this need to be exported?  This is not a public header; I don't think we should be exporting it...  Please use LOCAL_INCLUDES if you need to include it from somewhere in Gecko (from where?).

>+++ b/content/canvas/src/CanvasGradient.h
>-#define NS_CANVASGRADIENTAZURE_PRIVATE_IID \
>-    {0x28425a6a, 0x90e0, 0x4d42, {0x9c, 0x75, 0xff, 0x60, 0x09, 0xb3, 0x10, 0xa8}}

This is a bit dangerous if anyone ever does:

  nsCOMPtr<CanvasGradient> foo = do_QueryInterface(bar);

I guess we hope no one ever does...

>+++ b/content/canvas/src/CanvasRenderingContext2D.cpp

The changes here are awesome!

>+++ b/content/html/content/src/HTMLCanvasElement.cpp
>+#include "mozilla/dom/UnionTypes.h"

Why is this needed?

>+++ b/dom/bindings/Codegen.py
>@@ -718,54 +719,57 @@ def UnionTypes(descriptors, dictionaries
>-                            declarations.add((typeDesc.nativeType, False))
>-                            implheaders.add(typeDesc.headerFile)
>+                            headers.add(typeDesc.headerFile)

This will make UnionTypes.h include the world, eventually.  Why do we want that?  Why can't we get away with just forward-declaring these interface types?

>@@ -2516,17 +2520,18 @@ def getJSToNativeConversionInfo(type, de
>+                                isUnionReturnValue=False):

How about isInUnionReturnValue instead?

>@@ -3050,17 +3055,18 @@ for (uint32_t i = 0; i < length; ++i) {
>         forceOwningType = ((descriptor.interface.isCallback() and

Please adjust the comment above this code.

This is not doing anything special for isSpiderMonkeyInterface cases, which means they won't work, but more on this below.

>@@ -4290,17 +4298,17 @@ def getRetvalDeclarationForType(returnTy
>     if returnType.isUnion():
>+        return CGGeneric(returnType.unroll().name + "ReturnValue"), True, None, None

This is going to fail if returnType.nullable().  And in fact, if I do that I get a C++ compilation error, because getWrapTemplateForType assumes it'll be a pointer or something silly like that.  

I suggest doing the following:

1)  Add tests to TestCodeGen.webidl for the "(object or long)" union return value
    and the "(object or long)?" union return value.
2)  Fix things so they compile.  ;)

It might be good to add tests for this stuff to TestJSImplGen and TestExampleGen as well... and then comment them out, because they will totally fail.  Followup is OK on fixing them.

>+class CGUnionReturnValueStruct(CGThing):

The copy/paste from CGUnionStruct is a bit unfortunate...  we should really redo this with CGClass or something so we can share more stuff.  Followup?

>+        if self.type.hasNullableType:

As far as I can tell you never generate a setter to set to null in this case.  Please fix that.

>+        # Now have to be careful: we do not want the SetAsObject() method!

Then why are we allowing these things to contain "object" at all?

I don't think we should, because we can't support it in the current setup.  Please make this code throw if any of the types is "object".  We can't do "any" either, but luckily that's not allowed in unions anyway.  The other thing that we can't do is spidermonkey interfaces, since for return values those are represented as JSObject*.  This code should throw an error on those too.

Also, can you remove the setterTemplate bits altogether from CGUnionStruct?  They seem to be unused there, unsurprisingly.

>+        setterTemplate = """ ${structType}& SetAs${name}()

This actually needs one more space after the """.

>+                                   "      rval.set(JS::NullValue());\n"

rval.setNull().  Yes, I know you copied it; feel free to fix the other copy too.  ;)

>+        if type.isSpiderMonkeyInterface():

This can go away, since we don't support those here anyway.

>+                "objectCanBeNonNull": True

This can go away, afaict, it's unused.  Similar for the thing in CGUnionStruct.

r=me with the above fixed.  If you can't fix the include thing, please don't just check in; we should figure out how to make that work.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Oh, and file a followup on the fact that Date can't be used in a union, low-priority as that is?  ;)

Comment 19

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Comment on attachment 762533 [details] [diff] [review]
> Updated patch
> 
> >+++ b/content/base/src/moz.build
> >+    'nsImageLoadingContent.h',
> 
> Why does this need to be exported?  This is not a public header; I don't
> think we should be exporting it...  Please use LOCAL_INCLUDES if you need to
> include it from somewhere in Gecko (from where?).
> 

This was getting pulled in to nsJSEnvironemnt.cpp.  I changed to a LOCAL_INCLUDE.

> 
> >+++ b/content/html/content/src/HTMLCanvasElement.cpp
> >+#include "mozilla/dom/UnionTypes.h"
> 
> Why is this needed?
> 

We use HTMLImageOrCanvasOrVideoElement in HTMLCanvasElement::CopyInnerTo.

> >+++ b/dom/bindings/Codegen.py
> >@@ -718,54 +719,57 @@ def UnionTypes(descriptors, dictionaries
> >-                            declarations.add((typeDesc.nativeType, False))
> >-                            implheaders.add(typeDesc.headerFile)
> >+                            headers.add(typeDesc.headerFile)
> 
> This will make UnionTypes.h include the world, eventually.  Why do we want
> that?  Why can't we get away with just forward-declaring these interface
> types?


Because when we call SetValue on OwningNonNull we end up calling release.

> 
> >@@ -2516,17 +2520,18 @@ def getJSToNativeConversionInfo(type, de
> >+                                isUnionReturnValue=False):
> 
> How about isInUnionReturnValue instead?
> 

Sure, that's better.

> 
> >+class CGUnionReturnValueStruct(CGThing):
> 
> The copy/paste from CGUnionStruct is a bit unfortunate...  we should really
> redo this with CGClass or something so we can share more stuff.  Followup?

Bug 883493.

> 
> Also, can you remove the setterTemplate bits altogether from CGUnionStruct? 
> They seem to be unused there, unsurprisingly.
> 

What?  They are used.  nsJSEventListener uses EventOrString and calls the setters.

The comments that I haven't responded to were either trivial to fix or I haven't addressed them yet.  Attaching a WIP so I don't lose work.

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: WIP v2

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Because when we call SetValue on OwningNonNull we end up calling release.

Hrm.  Can we out-of-line this or something?  :(  Having UnionTypes include the world like that is really bad.

> What?  They are used.  nsJSEventListener uses EventOrString and calls the setters.

Ah, I see.  I just saw they were unused in bindings code.  :(

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: Codegen.py changes

These are the changes to Codegen.py.  I ended up changing or rebasing all the changes to this file so interdiff wouldn't be that useful.

Comment 23

[David Zbarsky (:dzbarsky)]

Attachment: Add tests

Comment 24

[David Zbarsky (:dzbarsky)]

Comment on attachment 773579 [details] [diff] [review]
Add tests

Hmm, that nullable union return value test isn't right.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 773510 [details] [diff] [review]
Codegen.py changes

>@@ -727,32 +728,36 @@ def UnionTypes(descriptors, dictionaries
>+            if not any(member.isObject() or member.isSpiderMonkeyInterface() for member in t.flatMemberTypes):
>+                unionReturnValues[name] = CGUnionReturnValueStruct(t, providers[0])

This could use a comment explaining why the check is there...

>@@ -4316,17 +4328,17 @@ def getRetvalDeclarationForType(returnTy
>+        return CGGeneric(returnType.unroll().name + "ReturnValue"), True, None, None

I still don't see how this can work for nullable union return values....

>+++ b/dom/bindings/parser/WebIDL.py
>+            if self.type.nullable():
>+                raise WebIDLError("An attribute cannot be of a nullable union"
>+                                  "type", [self.location])

Why can't it be?

Comment 26

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #25)
> Comment on attachment 773510 [details] [diff] [review]
> Codegen.py changes
> 
> 
> >@@ -4316,17 +4328,17 @@ def getRetvalDeclarationForType(returnTy
> >+        return CGGeneric(returnType.unroll().name + "ReturnValue"), True, None, None
> 
> I still don't see how this can work for nullable union return values....
> 

I still need to implement this.

> >+++ b/dom/bindings/parser/WebIDL.py
> >+            if self.type.nullable():
> >+                raise WebIDLError("An attribute cannot be of a nullable union"
> >+                                  "type", [self.location])
> 
> Why can't it be?

We throw an error further in the parser anyway, so I just disallowed them for now.  I can try fixing instead if we care.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> We throw an error further in the parser anyway, so I just disallowed them for now.

Where do we throw the error?

Comment 28

[David Zbarsky (:dzbarsky)]

Attachment: Interdiff

I fixed attributes and nullable union return values.  Also posting the full patch because I'm not quite sure what this interdiff is against (I had about 4 different versions of this patch in various trees).

Comment 29

[David Zbarsky (:dzbarsky)]

Attachment: WIP v3

Comment 30

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #25)
> Comment on attachment 773510 [details] [diff] [review]
> Codegen.py changes
> 
> >@@ -727,32 +728,36 @@ def UnionTypes(descriptors, dictionaries
> >+            if not any(member.isObject() or member.isSpiderMonkeyInterface() for member in t.flatMemberTypes):
> >+                unionReturnValues[name] = CGUnionReturnValueStruct(t, providers[0])
> 
> This could use a comment explaining why the check is there...
> 

Forgot to do this before uploading.  Fixed locally.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 780230 [details] [diff] [review]
Interdiff

>+            inner = self.type.inner if self.type.nullable() else self.type
>+            for f in inner.flatMemberTypes:

Can this just be:

  for f in self.type.unroll().flatMemberTypes:

?  The rest looks fine, assuming you add the relevant lines to TestJSImplGen.webidl/TestExampleGen.webidl

Comment 32

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #31)
> Comment on attachment 780230 [details] [diff] [review]
> Interdiff
> 
> >+            inner = self.type.inner if self.type.nullable() else self.type
> >+            for f in inner.flatMemberTypes:
> 
> Can this just be:
> 
>   for f in self.type.unroll().flatMemberTypes:
> 
> ?  

Ah, so that's what unroll does ;)

The rest looks fine, assuming you add the relevant lines to
> TestJSImplGen.webidl/TestExampleGen.webidl

The example gen fails for now.  We can fix in a followup.

https://hg.mozilla.org/integration/mozilla-inbound/rev/889041639eb9

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/889041639eb9