Implement WebIDL union return values

RESOLVED FIXED in mozilla25

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 742221 [details] [diff] [review]
Patch
Attachment #742221 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

5 years ago
Created attachment 742223 [details] [diff] [review]
Patch
Attachment #742221 - Attachment is obsolete: true
Attachment #742221 - Flags: review?(bzbarsky)
Attachment #742223 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #742223 - Attachment is patch: true
Attachment #742223 - Attachment mime type: message/rfc822 → text/plain
(Assignee)

Comment 2

5 years ago
Created attachment 742224 [details]
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]

Updated

5 years ago
Blocks: 858217, 768069
(Assignee)

Updated

5 years ago
Duplicate of this bug: 858217
(Assignee)

Updated

5 years ago
Duplicate of this bug: 768069
(Assignee)

Updated

5 years ago
Blocks: 580070
No longer blocks: 768069, 858217
(Assignee)

Comment 6

5 years ago
Created attachment 742429 [details] [diff] [review]
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.
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...)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 12

5 years ago
That would be nice.  Did smaug ever finish that patch?
No, but I think we should do it....
Attachment #742429 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 748502 [details] [diff] [review]
Patch

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
(Assignee)

Comment 16

5 years ago
Created attachment 762533 [details] [diff] [review]
Updated patch
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?  ;)
(Assignee)

Comment 19

5 years ago
(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.
> 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.  :(
(Assignee)

Comment 22

5 years ago
Created attachment 773510 [details] [diff] [review]
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.
Attachment #773510 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

5 years ago
Created attachment 773579 [details] [diff] [review]
Add tests
Attachment #773579 - Flags: review?(bzbarsky)
(Assignee)

Comment 24

5 years ago
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-
(Assignee)

Comment 26

5 years ago
(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?
(Assignee)

Comment 28

5 years ago
Created attachment 780230 [details] [diff] [review]
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).
Attachment #780230 - Flags: review?(bzbarsky)
(Assignee)

Comment 29

5 years ago
Created attachment 780231 [details] [diff] [review]
WIP v3
Attachment #762533 - Attachment is obsolete: true
Attachment #773405 - Attachment is obsolete: true
Attachment #773510 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
(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+
(Assignee)

Comment 32

5 years ago
(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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.