918011 - Support dictionaries inside unions

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This will work as long as the dictionaries inside the union are in separate webidl files.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move RootedDictionary into its own header.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Support dictionaries in unions.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

This needs some tests.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Good news: writing the tests found some bugs.

Bad news: that means another patch to review.

Good news: The resulting code is way cleaner.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Preprocess some of our test WebIDL files so we can have debug-only tests.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Get rid of isInOwningUnion: it's equivalent to another isMember value.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from the old part 2 to the new part 4, for review ease

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Support dictionaries in unions.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 17

[Nathan Froyd [:froydnj]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fixed the nits, and:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45e3854df7fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da382c02bcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7192af15890
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea71558fef5b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45e3854df7fe
https://hg.mozilla.org/mozilla-central/rev/2da382c02bcf
https://hg.mozilla.org/mozilla-central/rev/a7192af15890
https://hg.mozilla.org/mozilla-central/rev/ea71558fef5b

Comment 20

[Boris Zbarsky [:bzbarsky]]

Documented-ish at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types