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

Update the presentation-api IDL file #9814

Conversation

@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 5, 2018

Hello reviewer(s),

This PR is intended to consolidate the spec’s IDL definition with the WPT test suite’s copy, and any idlharness tests.

The up-to-date copy of the IDL file was automatically extracted from the reffy-reports repo (https://github.com/tidoust/reffy-reports/tree/master/whatwg/idl) which scrapes known specs automatically + regulary.

This PR is part of a migration project which will eventually be automatically updating and creating PRs for changes in spec IDL.

Please check that:
The Spec (and its source) is correct and up-to-date
All tests which cover the IDL in the spec have been migrated to fetch + use the idl in the interfaces/ directory (instead of inline copies in the test files).

@wpt-pr-bot wpt-pr-bot requested review from jensl and yuki3 Mar 5, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 5, 2018

Build PASSED

Started: 2018-03-08 04:47:37
Finished: 2018-03-08 04:56:33

View more information about this build on:

Luke Bjerring

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-presentation-api branch from f076ed6 to 8359cb3 Mar 7, 2018

@tomoyukilabs

This comment has been minimized.

Copy link
Contributor

tomoyukilabs commented Jun 1, 2018

@tidoust @louaybassbouss Could you review this PR? Note that:

  • The purpose of this PR seems to be mentioned in #9908 (comment).
  • The IDL file in this PR contains all interfaces for both controlling and receiving UAs. However, the current Presentation API test suite contains the separated IDL tests.

lukebjerring added some commits Jun 5, 2018

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Jun 6, 2018

@tomoyukilabs - thanks for pointing out those IDL tests. Updated them to use the imported IDL, and fixed up the deps too. PTAL.

@lukebjerring lukebjerring requested a review from foolip Jun 18, 2018

@foolip

foolip approved these changes Jun 18, 2018

@lukebjerring lukebjerring added this to In progress in Auto-import IDL files Jun 18, 2018

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Jun 18, 2018

@tidoust @louaybassbouss - are you happy for me to land this?

@lukebjerring lukebjerring moved this from In progress to To do in Auto-import IDL files Jun 18, 2018

@lukebjerring lukebjerring moved this from To do to In progress in Auto-import IDL files Jun 18, 2018

@tidoust

This comment has been minimized.

Copy link
Contributor

tidoust commented Jun 19, 2018

There's one thing specific about the Presentation API that @tomoyukilabs pointed out and that seems worth emphasizing: the Presentation API defines two conformance classes (controlling UA and receiving UA), and the IDL that needs to be supported is different for each of them.

The presentation-api.idl file contains the entire IDL defined in the specification. Some of it applies to controlling UAs, some of it to receiving UAs. The conformance section lists the interfaces that need to be supported by each class.

The presentation-api.idl file created by this PR will check controlling UAs. Depending on the implementation, the IDL harness may report failures for the PresentationReceiver and PresentationConnectionList interfaces, which do not need to be supported in controlling UAs (tests will pass in Chrome because it supports both classes, but that's not a requirement). These failures would be false positives.

To test the IDL in a receiving UA, one needs to run the IDL harness within a receiving context. The corresponding (manual) test is in: receiving-ua/idlharness-manual.https.html. That file and its dependencies are not touched by this PR, and that's a good thing for now. But that also means the receiving IDL is still being maintained manually (in receiving-ua/support/idlharness_receiving-ua.html).

All in all, I believe we can merge as-is, although there are two things that we might want to do (either now or later on):

  1. Make the receiving test mentioned above use the presentation-api.idl file to avoid having to maintain the IDL manually. If we do that, the test will also report false positives, because receiving UAs do not have to implement all interfaces of controlling UAs.
  2. Implement custom IDL extraction rules for the Presentation API to create a presentation-api-controlling-ua.idl file and a presentation-api-receiving-ua.idl file. This way, implementers wouldn't have to filter out individual IDL failures to ignore those that do not apply to them, they would simply be able to ignore the IDL file as a whole. Now, the problem with custom rules is that the list of interfaces that compose each file would need to be hardcoded and maintained somewhere (well, these lists could perhaps be generated from the conformance section).

@lukebjerring I'm not sure what would make implementers' life easier. Would having separated IDL files be preferable, or is it fine to maintain a list of false positives?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Jun 19, 2018

We have a function param which supports filtering only some of the IDL (blacklist or whitelist both supported). Happy to set the test up that way.

How would I tell what's what? By looking at the inlined IDL before my changes?

EDIT: Yeah that's exactly how I could tell, so have excluded PresentationRequest and PresentationAvailability from receiving-ua, and excluded PresentationReceiver and PresentationConnectionList from controlling-ua. @tidoust is that what you expected?

Luke Bjerring

@lukebjerring lukebjerring merged commit a7864de into web-platform-tests:master Jun 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Auto-import IDL files automation moved this from In progress to Done Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.