Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out what to do about optional dictionary members that are also dictionaries #23640

Closed
Manishearth opened this issue Jun 27, 2019 · 0 comments
Closed
Labels

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jun 27, 2019

Our last webidl update explicitly excludes bug 1368949 (see also: bug 1493860 , bug 1493798) This basically means that the following doesn't work:

dictionary Foo {required bool thing; };
dictionary Bar {Foo foo;}

since we treat null=empty for dictionary members, which doesn't quite work for dictionaries that can't be empty

This is also related to heycam/webidl#76

It's a bit of a mess, I tried to fix it and ended up realizing that I'm not quite sure what the fix should be (just applying the patch from bug 1368949 isn't enough, since our media and domquad code breaks. The Gecko patches don't seem to distinguish between dictionaries with required members and those without, so codegen overcorrects.

Ideally, we continue to handle the following case the same way:

dictionary Foo {bool thing = false; };
dictionary Bar {Foo foo;}

with an unspecified foo defaulting to {thing: false}, but if the member is required, then we instead create a nullable field.

A simple fix is to apply the patch and just change our DOM code to accept the spuriously generated Options in a couple places.

bors-servo added a commit that referenced this issue Jun 29, 2019
Improve support for nested dictionaries

Fixes #23640

Some IDLs need `= null`, that's something that needs to be updated upstream too.

After talking with @bzbarsky I realized that it was our IDLs which were incorrect, causing Options to appear where we don't want them to. In the media code we _do_ want Options. `= null` is the correct fix for that (and should be upstreamed).

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23653)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant
You can’t perform that action at this time.