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

Use interface mixins instead of [NoInterfaceObject] #3207

Merged
merged 5 commits into from
Dec 20, 2017

Conversation

romandev
Copy link
Contributor

@romandev romandev commented Nov 7, 2017

@romandev
Copy link
Contributor Author

romandev commented Nov 7, 2017

@tobie Could you check that this patch is going on the right direction?

@tobie
Copy link
Contributor

tobie commented Nov 7, 2017

It is. :) You can track your work here, if needed: #3194.

@annevk
Copy link
Member

annevk commented Nov 7, 2017

Thanks @romandev! We might also be able to reduce some [Exposed] usage as mixins inherit that from whatever they're included into (when it's more restrictive it'll have to stay).

As noted in the issue @tobie points to we might have to wait with landing this for a while until we know that all tooling is updated. Hope that's understandable.

@romandev
Copy link
Contributor Author

romandev commented Nov 7, 2017

@tobie, @annevk Thank you for your comments!

@annevk I'm first-time contributor, so, I didn't fully understand your context.
If you don't mind, could you let me know what kind of tools you mentioned?
(e.g. webidl2? or binding implementation such as Chromium binding?)

Thank you!

@annevk
Copy link
Member

annevk commented Nov 7, 2017

@romandev relevant here are https://github.com/plinss/widlparser and https://github.com/tabatkins/bikeshed I think. Those are used to scrape the HTML Standard and extract all the IDL fragments which are then put in a database so other specifications can link to them.

@annevk
Copy link
Member

annevk commented Nov 7, 2017

To be clear, that doesn't mean your continued work here is not valuable. It just means we have to wait a bit before applying the patch to master.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main outstanding problem here is that we still need to update idlharness.js.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Dec 17, 2017
@annevk
Copy link
Member

annevk commented Dec 18, 2017

It seems that was already updated. @romandev do you want to add your name to the Acknowledgments section?

@tobie
Copy link
Contributor

tobie commented Dec 18, 2017

While the syntax parser used by idlharness was updated, idlharness itself wasn't yet (see web-platform-tests/wpt#8054). I don't think [NoInterfaceObject] interfaces were tested before, so I don't think this will make a huge difference, unless it actually breaks stuff and throws errors.

@annevk
Copy link
Member

annevk commented Dec 18, 2017

Okay, I just merged a couple things... But I think you're right that if it wasn't tested before this should essentially no-op. Would be good to get those tested as well of course.

@tobie
Copy link
Contributor

tobie commented Dec 18, 2017

Would be good to get those tested as well of course.

I concur.

@romandev
Copy link
Contributor Author

@annevk, @tobie Thank you for review. BTW, Are you working on fixing idlharness.js? I'm also working on it now. If you are working on it, please let me know. I can stop the work :)

@annevk
Copy link
Member

annevk commented Dec 18, 2017

I'm not.

@tobie
Copy link
Contributor

tobie commented Dec 18, 2017

Neither am I. :)

WebIDL recently introduced dedicated syntax for mixins[1]. So, we can
replace `[NoInterfaceObject]` and `implements` with `interface mixin` and
`includes`.

[1] whatwg/webidl@45e8173
@domenic
Copy link
Member

domenic commented Dec 20, 2017

Given that other specs are already merging these I think we should merge it for HTML too. Fixing idlharness.js will be necessary before we can properly merge corresponding WPT PRs, but that's separable.

I'll do a last pass-through to check and then merge.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Dec 20, 2017
@annevk
Copy link
Member

annevk commented Dec 20, 2017

Note that @romandev updated idlharness in web-platform-tests/wpt#8727 to make it equally functional to the old version. A new PR is still required to actually test mixin members though.

@domenic domenic merged commit c8867a1 into whatwg:master Dec 20, 2017
foolip added a commit that referenced this pull request Jan 21, 2019
annevk pushed a commit that referenced this pull request Jan 21, 2019
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants