Closed Bug 1068740 Opened 6 years ago Closed 5 years ago

Consider putting union types in the binding files where they're used


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

Not set





(Reporter: bzbarsky, Assigned: peterv)


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



(2 files, 4 obsolete files)

Specifically, doing this for union types that are only used in one binding file.

That will help with issues like bug 1068491, within certain limits (e.g. a dictionary containing a union member that contains another dictionary defined in the same webidl file could still be a problem, unless we enhance our dictionary-sorting algorithm to interleave unions and dictionaries).

This is not that difficult to do; for any given union type instance, CGHeaders.getDeclarationFilename will tell you the right header to put it in, for example, so you could check that for a given type those all match up.

An issue would be that the right header to include for the union would magically change if it started being used elsewhere...

We could try to avoid that by requiring that unions with the same underlying value set used from multiple webidl files be used via typedefs, so that the actual union type has a canonical location it lives, but that reduces our ability to just copy/paste IDL from specs.

Alternately, we could try to put each union type into its own header or some such... but that would actually not help the case of bug 1068491 unless we did that with each enum and dictionary too.

I now understand why some people like having structural types in their language... ;)
(In reply to Boris Zbarsky [:bz] from comment #0)
> An issue would be that the right header to include for the union would
> magically change if it started being used elsewhere...

Well, they'd be including the binding header where the union is defined. If we start including UnionTypes.h from that header once the union is used elsewhere then things would still work? The problem would be if someone includes UnionTypes.h and then the union is removed from a WebIDL file so that it's only used in one place anymore. That's going to be rare I bet (and we should discourage including UnionTypes.h).
I have this somewhat working, but typedefs complicate things, because that type's filename will be where the typedef is. For deciding which header to include for a union we'll probably have to store some metadata on the uniontypes that notes whether they also exists in other files.
> because that type's filename will be where the typedef is.

Why is that a problem?   If we have:

     typedef (x or y) z;
     // use z


     // use z

then yes, both will see the filename as ABinding.h, but that seems fine to me...
Attached patch v1 (obsolete) — Splinter Review
This seems to mostly work.
Assignee: nobody → peterv
Attached patch v1 (obsolete) — Splinter Review
Attachment #8498826 - Attachment is obsolete: true
Thanks for the patch. I get this error though:

In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dom/bindings/UnifiedBindings9.cpp:34:
In file included from ./MediaKeySessionBinding.cpp:10:
../../dist/include/mozilla/dom/MediaKeySession.h:66:51: error: unknown type name 'ArrayBufferViewOrArrayBuffer'
                                            const ArrayBufferViewOrArrayBuffer& aInitData,
That's just because you have bug 1059043 in your tree already, moving the change to add |class ArrayBufferViewOrArrayBuffer;| from content/media/eme/MediaKeys.h to content/media/eme/MediaKeySession.h should work.
Attached patch v1 (obsolete) — Splinter Review
Merged to inbound.
Attachment #8498830 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
hg qref helps.
Attachment #8499460 - Attachment is obsolete: true
Attached patch v1.1Splinter Review
Attachment #8499474 - Attachment is obsolete: true
Attachment #8500669 - Flags: review?(bzbarsky)
Comment on attachment 8500669 [details] [diff] [review]

>+++ b/dom/bindings/
>+def UnionsForFile(config, webIDLFile):

This has a more complicated return value than documented; it would be good to document what it actually returns.

>+def UnionTypes(unionTypes, config):

Please document the form of the unionTypes argument, since it's a bit non-obvious.

>+def UnionConversions(unionTypes, config):

And here.

>+        cgthings.extend(traverseMethods)
>+        cgthings.extend(unlinkMethods)

What guarantees that those lists are in stable order?

>+        def getDependenciesFromType(type):
>+            if type.isDictionary():
>+                return set([type.unroll().inner])

So this returns a set containing a single dictionary (not dictionary type; an actual IDLDictionary).

>+            if type.isUnion():
>+                return set([type.unroll()])

But this returns a set containing a union type?  Could we please make this consistently return types or something?

>+        def getDependencies(type):

The argument to this is not always a type, because of what getDependenciesFromType does and what callers pass in.

>+            if type.isDictionary():

So this is using IDLObject.isDictionary or IDLDictionary.isDictionary... or IDLType.isDictionary or isDictionary on one of the subclasses of IDLType.  Again the confusion between objects and types is not great.

Unfortunately, getting to an IDLWrapperType for a dictionary seems like a PITA.  So I'm not sure what the best solution is here; maybe it's just better naming and more comments.

>+        for clazz, isStruct in unionDeclarations:

How about "className" instead of "clazz"?  This comes up in a few places.

>     def UnionTypes(config):
>+        unions = CGList(traverseMethods +
>+                        unlinkMethods +
>+                        [CGUnionStruct(t, config.getDescriptorProvider(False)) for t in unionStructs] +
>+                        [CGUnionStruct(t, config.getDescriptorProvider(False), True) for t in unionStructs],

What does the deterministic sort here?

>+++ b/dom/bindings/
>+        self.filenamesPerUnion = defaultdict(set)
>+        self.unionsPerFilename = defaultdict(list)

Would be good to document these.

r=me with the above fixed.  Thank you!
Attachment #8500669 - Flags: review?(bzbarsky) → review+
Attached patch Additional patchSplinter Review
Boris, could you check that this does what you asked? Thanks.
Attachment #8504717 - Flags: feedback?(bzbarsky)
Comment on attachment 8504717 [details] [diff] [review]
Additional patch

Attachment #8504717 - Flags: feedback?(bzbarsky) → feedback+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1103153
Component: DOM → DOM: Core & HTML
Blocks: 1584009
You need to log in before you can comment on or make changes to this bug.