Support dictionaries inside unions

RESOLVED FIXED in mozilla27

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla27
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

This will work as long as the dictionaries inside the union are in separate webidl files.
Created attachment 806834 [details] [diff] [review]
part 1.  Move RootedDictionary into its own header.
Attachment #806834 - Flags: review?(bugs)
Created attachment 806836 [details] [diff] [review]
part 2.  Support dictionaries in unions.
Attachment #806836 - Flags: review?(bugs)

Comment 3

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #0)
> This will work as long as the dictionaries inside the union are in separate
> webidl files.

Somewhat odd limitation. What happens if dictionaries are in the same file?

Comment 4

5 years ago
Comment on attachment 806834 [details] [diff] [review]
part 1.  Move RootedDictionary into its own header.

Uh, painful diff. I guess it is right.
Stuff copied to RootedDictionary.h, remove from the original file and
stuff in the original file removed from RootedDictionary.h
Attachment #806834 - Flags: review?(bugs) → review+

Comment 5

5 years ago
Comment on attachment 806836 [details] [diff] [review]
part 2.  Support dictionaries in unions.

This needs some tests.
Attachment #806836 - Flags: review?(bugs) → review-
> What happens if dictionaries are in the same file?

In the case when dictionary X has a union containing dictionary Y in it and X and Y are in the same Foo.webidl, FooBinding.h will include UnionTypes.h which will try to include FooBinding.h, fail due to include guards, and then sizeof(Y) will be unknown in UnionTypes.h and declaring the union will fail and the compile will fail.  With a possibly-hard-to-understand-if-you-haven't-read-this-comment error message.  :(

> Uh, painful diff.

Sorry.  Wanted to preserve the blame.  :(

> This needs some tests.

I did a bunch of manual testing.  Adding codegen tests would link the resulting code into release builds, which is undesirable.   Our union-testing situation sucks.
Flags: needinfo?(bugs)

Comment 7

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
> > What happens if dictionaries are in the same file?
> 
> In the case when dictionary X has a union containing dictionary Y in it and
> X and Y are in the same Foo.webidl, FooBinding.h will include UnionTypes.h
> which will try to include FooBinding.h, fail due to include guards, and then
> sizeof(Y) will be unknown in UnionTypes.h and declaring the union will fail
> and the compile will fail.  With a
> possibly-hard-to-understand-if-you-haven't-read-this-comment error message. 
> :(
I there anything we could do here. At least give some sane error message. It would be rather
natural to put dictionaries to the same file and use them there in an union.



> Sorry.  Wanted to preserve the blame.  :(
Yes, blame is more important than easy review.

> I did a bunch of manual testing.  Adding codegen tests would link the
> resulting code into release builds, which is undesirable.   Our
> union-testing situation sucks.
Hmm, would debug-build only tests be ok?
Flags: needinfo?(bugs)
> At least give some sane error message.

I can probably hack in something like that.  Let me try.

> would debug-build only tests be ok?

Hmm.  Yes.  I guess I can add some debug-only test IDL...
Good news: writing the tests found some bugs.

Bad news: that means another patch to review.

Good news: The resulting code is way cleaner.
Created attachment 807990 [details] [diff] [review]
part 2.  Preprocess some of our test WebIDL files so we can have debug-only tests.
Attachment #807990 - Flags: review?(khuey)
Created attachment 807992 [details] [diff] [review]
part 3.  Get rid of isInOwningUnion: it's equivalent to another isMember value.
Attachment #807992 - Flags: review?(bugs)
Created attachment 807994 [details] [diff] [review]
Interdiff from the old part 2 to the new part 4, for review ease
Created attachment 807995 [details] [diff] [review]
part 4.  Support dictionaries in unions.
Attachment #807995 - Flags: review?(bugs)
Attachment #806836 - Attachment is obsolete: true
Comment on attachment 807995 [details] [diff] [review]
part 4.  Support dictionaries in unions.

what happens for example if one does (EventInit or MutationObserverInit) ?
I assume some error.
Attachment #807995 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #807992 - Flags: review?(bugs) → review+
> what happens for example if one does (EventInit or MutationObserverInit) ?

WebIDL validation error: dictionary types are not distinguishable, so you can't use two of them in a union.
Comment on attachment 807990 [details] [diff] [review]
part 2.  Preprocess some of our test WebIDL files so we can have debug-only tests.

Review of attachment 807990 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if nfroyd didn't bit rot this.

::: dom/webidl/moz.build
@@ +532,5 @@
> +        'TestCodeGen.webidl',
> +        'TestExampleGen.webidl',
> +        'TestJSImplGen.webidl',
> +    ]
> +    

nit: whitespace on empty line
Attachment #807990 - Flags: review?(khuey) → review+
Comment on attachment 807990 [details] [diff] [review]
part 2.  Preprocess some of our test WebIDL files so we can have debug-only tests.

Review of attachment 807990 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +23,5 @@
>      PreprocessedWebIDLFile,
>      Program,
>      ReaderSummary,
>      TestWebIDLFile,
> +    PreprocessedTestWebIDLFile,

Nit: alphabetize.
Documented-ish at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types
Flags: needinfo?(bzbarsky)
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 929512
No longer blocks: 929512
Depends on: 929512
See Also: → bug 1033554
You need to log in before you can comment on or make changes to this bug.