Closed
Bug 1068740
Opened 10 years ago
Closed 10 years ago
Consider putting union types in the binding files where they're used
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bzbarsky, Assigned: peterv)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
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... ;)
Reporter | ||
Updated•10 years ago
|
Blocks: ParisBindings
Assignee | ||
Comment 1•10 years ago
|
||
(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).
Assignee | ||
Comment 2•10 years ago
|
||
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.
Reporter | ||
Comment 3•10 years ago
|
||
> because that type's filename will be where the typedef is.
Why is that a problem? If we have:
A.webidl:
typedef (x or y) z;
// use z
B.webidl:
// use z
then yes, both will see the filename as ABinding.h, but that seems fine to me...
Assignee | ||
Comment 4•10 years ago
|
||
This seems to mostly work.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8498826 -
Attachment is obsolete: true
Comment 6•10 years ago
|
||
Thanks for the patch. I get this error though:
Unified_cpp_protocol_data0.o
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,
Assignee | ||
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
Merged to inbound.
Attachment #8498830 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8499474 -
Attachment is obsolete: true
Attachment #8500669 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8500669 [details] [diff] [review]
v1.1
>+++ b/dom/bindings/Codegen.py
>+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/Configuration.py
>+ 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+
Assignee | ||
Comment 12•10 years ago
|
||
Boris, could you check that this does what you asked? Thanks.
Attachment #8504717 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8504717 [details] [diff] [review]
Additional patch
r=me
Attachment #8504717 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•