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

Put IDL files in specname/interfaces.idl instead of interfaces/specname.idl #9026

Closed
foolip opened this issue Jan 13, 2018 · 10 comments
Closed

Comments

@foolip
Copy link
Member

foolip commented Jan 13, 2018

#5339 started splitting out standalone IDL files, and we have quite a few now in https://github.com/w3c/web-platform-tests/tree/master/interfaces. Yay!

I've come to think that we should have these in the spec directories instead, which I think would have these benefits:

  • When changing just the IDL files, the people in specname/OWNERS will notice
  • The issue of divergent spec shortname and directory name cannot come up. We now have interfaces/webrtc-pc.idl but webrtc/. (I did that, but it's still not great.)

@mdittmer @lukebjerring FYI

@foolip
Copy link
Member Author

foolip commented Jan 13, 2018

@zcorpan @annevk @domenic @ayg @Honry @soareschen, you are the people who have done the most poking in interfaces/, WDYT?

One downside that's come to mind is that there's some variety in how the idlharness.js tests are named, and idlharness.html is actually more common than interfaces.html. And the combination idlharness.html+interfaces.idl is less pleasing than interfaces.html+interfaces.idl. But I still think I prefer sprinkling the IDL files around.

@domenic
Copy link
Member

domenic commented Jan 13, 2018

Sounds like an improvement to me.

@Honry
Copy link
Contributor

Honry commented Jan 15, 2018

I'm happy on this improvement.

@annevk
Copy link
Member

annevk commented Jan 15, 2018

I'm not a big fan of the continued churn as each time you move files around you make git log harder to use. If we're sure this is the setup we're gonna use it might work though. Another thing that would be nice is that WPT generates the .html (and .worker, and .https. variants) automatically based on a .idl file. I guess that's going to be a bit further away though.

@foolip
Copy link
Member Author

foolip commented Jan 15, 2018

I'm not a big fan of the continued churn as each time you move files around you make git log harder to use. If we're sure this is the setup we're gonna use it might work though.

If you mean following the files that have been renamed, git log --follow does the trick, and git blame by default sees right past renames, including in the GitHub UI. As for the resulting changes in the interfaces.html files, well those won't be possible to skip.

To be honest, I'm actually not confident that this move would be the final state of things. It is the most obvious way to fix the OWNERS problem though.

Another thing that would be nice is that WPT generates the .html (and .worker, and .https. variants) automatically based on a .idl file. I guess that's going to be a bit further away though.

@mdittmer has talked about doing just that. We'd need to maintain a list "instance factories" to get objects for idl_array.add_objects, and those can sometimes be non-trivial as in webrtc/interfaces.https.html, but it's doable. It would be great to increase confidence that we're testing everything.

There is one possibility, also raised by @mdittmer, which is that we could split tests by interface instead of spec, by parsing all spec IDL and then serializing everything that has to do with Document, for example.

@annevk
Copy link
Member

annevk commented Jan 15, 2018

If I browse to https://github.com/w3c/web-platform-tests/blob/master/xhr/FormData-append.html and click on "History" I don't see history past renames. "Blame" does though, interestingly enough. Maybe I should ask GitHub if they want to fix it for "History" too.

@tobie
Copy link
Contributor

tobie commented Jan 15, 2018

It is the most obvious way to fix the OWNERS problem though.

Yeh, the other option would be allowing per file owners. This is a non-trivial piece of work, but might be interesting to tackle if there are other use cases around.

@foolip
Copy link
Member Author

foolip commented Mar 2, 2018

Turns out that we already have a mix of styles. Here are the IDL files outside of interfaces/:

  • IndexedDB/interfaces.idl
  • background-fetch/interfaces.idl
  • cookie-store/cookie-store.idl
  • encrypted-media/EncryptedMediaExtensions.idl
  • entries-api/interfaces.idl
  • html/infrastructure/common-dom-interfaces/collections/domstringlist.idl
  • storage/interfaces.idl
  • webauthn/interfaces.idl

Since @lukebjerring is now working on auto-updating IDL we should decide which direction to move in, since that will involve changing and creating a bunch of tests.

And there are 36 IDL files in interfaces/. We should pick a style and converge on that as part of the auto-update work that @lukebjerring has started.

After @annevk's skepticism I wasn't so sure of my proposal to just go ahead, but given the inconsistency we now have I think specname/interfaces.idl seems the most reasonable to align on.

Any objections? @gsnedders @jgraham?

@lukebjerring
Copy link
Contributor

IMHO, having a large number of interfaces.idl spattered everywhere drops a lot of implicit information about what the spec is, and makes it non-trivial to enumerate all of the spec/interface files.

The folder structure of interfaces/[spec-id].idl, and each IDL file beginning with a comment as to the canonical source URL would mean that we have a single place that attempts to exhaustively list the specs of the open web (or, at least the ones with IDL).

@foolip
Copy link
Member Author

foolip commented Mar 7, 2018

Yep, one directory will be easier for automation, which we've started now, so let's go with that. I'll move the outliers in #9026 (comment) into interfaces/ to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants