Closed Bug 865998 Opened 7 years ago Closed 7 years ago

Implement WebIDL union return values

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #742221 - Flags: review?(bzbarsky)
Attached patch Patch (obsolete) — Splinter Review
Attachment #742221 - Attachment is obsolete: true
Attachment #742221 - Flags: review?(bzbarsky)
Attachment #742223 - Flags: review?(bzbarsky)
Attachment #742223 - Attachment is patch: true
Attachment #742223 - Attachment mime type: message/rfc822 → text/plain
Attached file Generated code
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]
Duplicate of this bug: 858217
Duplicate of this bug: 768069
Blocks: ParisBindings
No longer blocks: 768069, 858217
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #742223 - Attachment is obsolete: true
Attachment #742223 - Flags: review?(bzbarsky)
Did you mean to ask for review on that?  (Note that I won't get to it until Monday at best...)
Attachment #742429 - Flags: review?(bzbarsky)
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 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?
It doesn't matter in this case because the context keeps them alive.  But yeah, I guess it should in general.
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.
That would be nice.  Did smaug ever finish that patch?
No, but I think we should do it....
Attachment #742429 - Flags: review?(bzbarsky) → review-
Attached patch Patch (obsolete) — Splinter Review
I switched to OwningNonNull for now.
Attachment #742429 - Attachment is obsolete: true
Attachment #748502 - Flags: review?(bzbarsky)
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
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #748502 - Attachment is obsolete: true
Attachment #748502 - Flags: review?(bzbarsky)
Attachment #762533 - Flags: review?(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.
Attachment #762533 - Flags: review?(bzbarsky) → review+
Oh, and file a followup on the fact that Date can't be used in a union, low-priority as that is?  ;)
Blocks: 883493
(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.
Attached patch WIP v2 (obsolete) — Splinter Review
> 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.  :(
Attached patch Codegen.py changes (obsolete) — Splinter Review
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.
Attachment #773510 - Flags: review?(bzbarsky)
Attached patch Add tests (obsolete) — Splinter Review
Attachment #773579 - Flags: review?(bzbarsky)
Comment on attachment 773579 [details] [diff] [review]
Add tests

Hmm, that nullable union return value test isn't right.
Attachment #773579 - Attachment is obsolete: true
Attachment #773579 - Flags: review?(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?
Attachment #773510 - Flags: review?(bzbarsky) → review-
(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.
> We throw an error further in the parser anyway, so I just disallowed them for now.

Where do we throw the error?
Attached patch InterdiffSplinter Review
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).
Attachment #780230 - Flags: review?(bzbarsky)
Attached patch WIP v3Splinter Review
Attachment #762533 - Attachment is obsolete: true
Attachment #773405 - Attachment is obsolete: true
Attachment #773510 - Attachment is obsolete: true
(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 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
Attachment #780230 - Flags: review?(bzbarsky) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/889041639eb9
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.