Open Bug 1045867 Opened 8 years ago Updated 2 years ago

Support renaming WebIDL union types

Categories

(Core :: DOM: Bindings (WebIDL), defect)

defect
Not set
normal

Tracking

()

ASSIGNED

People

(Reporter: jmorton, Assigned: peterv)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

It would be nice if the following WebIDL:

typedef (HTMLImageElement or
         HTMLVideoElement or
         HTMLCanvasElement or
         Blob or
         ImageData or
         CanvasRenderingContext2D or
         ImageBitmap) ImageBitmapSource;

Actually created a class called "ImageBitmapSource", and not a "HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap".

The problem with just typedefing this ridiculous name in C++ is that any forward declaration requires referencing the original name.

This can be very useful in instances where overloading is cumbersome, for example when we need to support 7 different types in two different methods on both the Window and WorkerGlobalScope. Without Union Types, 28 different methods would have to be created to implement just one method from the spec.

(this type of issue could also be resolved with something like blink/webkit's "heap supplements" so that WorkerGlobalScope and Window can use the same implementation)
Any objection Boris?
Flags: needinfo?(bzbarsky)
I have no objection to making unions suck less in general.

A few general comments:

1)  As I recall, we currently eagerly unroll IDL typedefs in the IDL parser and data model; the means the codegen doesn't have to worry about their existence at all.  But conversely by the time we're doing codegen we don't actually have the typedef; we just have the union type.

2)  The union type is named based on its members so that if the same union is used in several different places we can share it.  Maybe we can relax that in situations where the type is only used in one spot, basically; we don't really track that right now.

3)  We've considered putting different union types in different headers, so you could just include the one binding header you want for the union and not have to worry too much about forward-declaring.  This might be a bit painful to set up, though, given the sharing goal.

4)  Nothing is stopping you from sharing an implementation across Window and WorkerGlobalScope if it's shareable: put it on a class that both of them inherit from.  We do this with the constraint validation API for form controls, for example.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #2)

> 3)  We've considered putting different union types in different headers, so
> you could just include the one binding header you want for the union and not
> have to worry too much about forward-declaring.  This might be a bit painful
> to set up, though, given the sharing goal.

I would still have to use the 110 character name, though (and the header for it would have a 110 character file name).

My solution for now was to create a "ImageBitmapSource.h" which was just a "typedef ReallyLongName ImageBitmapSource" - so then any other header can just include that instead of forward declaring.

I suppose that if each union type was in its own header, then any aliases/typedefs could simply be their own header that includes the original type's header and then has a typedef for the alias. All of the actual implementations could still have the canonical name, then.

Aliases could also simply be subclasses of the canonical class, which would solve the forward declaration issues that typedefs have.

> 4)  Nothing is stopping you from sharing an implementation across Window and
> WorkerGlobalScope if it's shareable: put it on a class that both of them
> inherit from.  We do this with the constraint validation API for form
> controls, for example.

Yeah, I considered that, but wasn't sure if we wanted to add new superclasses to nsGlobalWindow for each new shared API. Maybe this is the best solution for my issue for now.
> Aliases could also simply be subclasses of the canonical class

Ah, that is a nice idea!  That ought to be pretty simple to codegen once we get the typedefs through to the codegen at all.  I'll try to take a look when I get back, but that's in 2.5 weeks.  If someone gets to it before me, I won't cry, obviously.  ;)

I think adding superclasses to nsGlobalWindow is just fine if it reduces code duplication.  That seems like it can happen independently from what we do with typedefs.
Flags: needinfo?(bzbarsky)
So I tried doing this.

It works sorta ok for some cases, but not for things like optional unions or sequences of unions, because templating on a subclass vs superclass is just not the same thing, unlike typedefs.  :(

I'll post what I have, but it has the drawback that you can _sometimes_ use the typedefed name, but not always.

We could fix this by changing the codegen for method calls to actually use the typedef if that's what was used in the IDL, but that's actually pretty annoying, because that requires keeping the typedef type associated with the argument or whatnot all the way through to that point.

Peter, are you interested in taking a patch for this given that constraint?
Flags: needinfo?(bzbarsky) → needinfo?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Oh, and the other option is in fact to put each union in its own header instead of having a catch-all UnionTypes.h and then just put typedefs in that header.  That still doesn't help with sane forward-declaration, but C++ is just not cooperating with us here.
Attachment #8519932 - Attachment description: Bug 1045867 part 0. Make DOM container types containing a union castable to the same container type containing a union typedef type. → part 0. Make DOM container types containing a union castable to the same container type containing a union typedef type.
This uses a python proxy class (adapted from http://code.activestate.com/recipes/496741-object-proxying/) to make typedefs transparent but detectable. I'm not sure the API is completely what we want. Detecting a typedef is done by calling .isTypedef(), .typedef then returns the IDLTypedefType. Getting the underlying type is possible through .typedef.inner. We could have a getter on the proxy for that (.aliasedType? and rename .typedef to alias?), but not sure it's really needed.
Assignee: bzbarsky → peterv
Attachment #8478427 - Attachment is obsolete: true
Attachment #8519934 - Flags: review?(bzbarsky)
Now with the right files :-/.
Attachment #8519932 - Attachment is obsolete: true
Attachment #8519932 - Flags: review?(bzbarsky)
Attachment #8519935 - Flags: review?(bzbarsky)
Comment on attachment 8519935 [details] [diff] [review]
part 0. Make DOM container types containing a union castable to the same container type containing a union typedef type.

Doing this on arbitrary types worries me.  Could we restrict the CastableDOMTypeBase behavior to cases when T and U both inherit from some base class for all unions and still be ok here?

r=me
Attachment #8519935 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519936 [details] [diff] [review]
part 1. Have codegen spit out union classes corresponding to typedefed names for the union; these can then be forward-declared and whatnot as desired.

Codegen.py:
>+            sorted(config.unionTypedefs[type]))

This sort of thing worries me.  What makes us think that equality on the type objects will do something sane here?

I'd much rather key on the name instead.

WebIDL.py:

Does __slots__ play nice with pickling?  Why do we need a __weakref__ slot?  Document, please.

>+    @property
>+    def inner(self):

Maybe document that this is not actually exposed to consumers but is for our own internal use?

>+        return self if unrolled == forwardTo else unrolled

Hmm, so unrolling a typedef for a nullable union type loses the typedefness?  I guess that's ok, since so does .inner, and there's no real way to represent it anyway.

>+    def __delattr__(self, name):
>+        if name == "typedefName":

Document why this is needed, as well as the similar thing in __setattr__?  Because I don't see why it's useful.

>+      creates an proxy instance referencing `obj`. (obj, *args, **kwargs) are

"a proxy"

>+++ b/dom/bindings/test/TestCodeGen.webidl

What about examplegen and jsimplgen?

And for that matter, what does example codegen generate for a typedefed union with this patch?

r=me modulo the above.  Thanks for picking this up!
Attachment #8519936 - Flags: review?(bzbarsky) → review+
Depends on: 1103153
Duplicate of this bug: 1250931
Component: DOM → DOM: Core & HTML
Component: DOM: Core & HTML → DOM: Bindings (WebIDL)
You need to log in before you can comment on or make changes to this bug.