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 web-animations IDL file #9908

Merged

Conversation

5 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 7, 2018

No description provided.

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

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-web-animations branch from de86bdf to 0e28af5 Mar 7, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 8, 2018

Build PASSED

Started: 2018-03-20 17:29:17
Finished: 2018-03-20 17:33:49

View more information about this build on:

Luke Bjerring added some commits Mar 19, 2018

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-web-animations branch from 8b81b8d to 419946f Mar 19, 2018

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 19, 2018

Relies on #10100 for the only filtering of untested IDL.

@lukebjerring lukebjerring requested a review from foolip Mar 19, 2018

@birtles

This comment has been minimized.

Copy link
Contributor

birtles commented Mar 19, 2018

Sorry, I'm missing some context. What is the purpose of this PR?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 20, 2018

Hi, sorry for the lack of context - we (@foolip, @mdittmer and I) are working on a pipeline which automatically scrapes known WebIDL sources/specs and updates the IDL in the interfaces whenever it changes.

This PR is the output of running the scraper as a one-off, for which I've included updating the existing/equivalent IDL snippets that are inlined in the tests, to instead reference the generated (singular) interfaces/web-animations.idl file.

Later, we'll use OWNERS to automatically ping relevant persons when the source changed and the scraped IDL differs from what's in WPT.

@birtles
Copy link
Contributor

birtles left a comment

Thanks for doing this but I think the scraper needs to be run again. It appears to be using an old version of the spec.

@@ -0,0 +1,177 @@
// GENERATED CONTENT - DO NOT EDIT
// Content of this file was automatically extracted from the Web Animations spec.
// See https://w3c.github.io/web-animations/

This comment has been minimized.

Copy link
@birtles

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 20, 2018

Author Contributor

I'll defer that change for @foolip

This comment has been minimized.

Copy link
@foolip

foolip Mar 20, 2018

Contributor

https://w3c.github.io/web-animations/ now redirects to https://drafts.csswg.org/web-animations/ but not using a HTTP redirect.

Sent two reviews to update URLs I found in Chromium, only the first actually relevant:
https://chromium-review.googlesource.com/c/chromium/src/+/970648
web-animations/web-animations-js#191

@lukebjerring, have the other recent changes taken effect in the output of the IDL scraper? If so, then after the first has landed, another update should fix this.

This comment has been minimized.

Copy link
@foolip

foolip Mar 20, 2018

Contributor

Ah, except the day-to-day has the wrong URL as well. Will fix.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Mar 20, 2018

Author Contributor

Updated the source (switched https://drafts.csswg.org/web-animations/ to be the canonical source and https://w3c.github.io/web-animations/ to be an alt source)

enum AnimationPlayState { "idle", "running", "paused", "finished" };

[Exposed=Window]
interface AnimationEffectReadOnly {

This comment has been minimized.

Copy link
@birtles

birtles Mar 20, 2018

Contributor

This interface no longer exists in the latest version of Web Animations.

};

[Exposed=Window]
interface AnimationEffectTimingReadOnly {

This comment has been minimized.

Copy link
@birtles

birtles Mar 20, 2018

Contributor

Nor does this interface.

};

[Exposed=Window]
interface AnimationEffectTiming : AnimationEffectTimingReadOnly {

This comment has been minimized.

Copy link
@birtles

birtles Mar 20, 2018

Contributor

Nor this one.

@birtles

This comment has been minimized.

Copy link
Contributor

birtles commented Mar 20, 2018

Also, one question about the GENERATED CONTENT - DO NOT EDIT part:

Often I'll make spec, test, and code changes simultaneously, update the tests/code in Gecko, then upstream the tests to wpt. Can I rerun the scraper locally so that I can land the updated tests along with the code in Gecko?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 20, 2018

Also, one question about the GENERATED CONTENT - DO NOT EDIT part:

NB: If you copy-paste from the source, strip leading whitespace, single blank line between snippets, your output will exactly match anyway. The comment is targeted at those who are unfamiliar with the spec, so as not to move the file.

Often I'll make spec, test, and code changes simultaneously, update the tests/code in Gecko, then upstream the tests to wpt. Can I rerun the scraper locally so that I can land the updated tests along with the code in Gecko?

Desired / future answer: Yes
Current answer: No

When the tool matures (it still has a few edge-cases with whitespace, URLs, etc) we'll standardize how it gets run, document it, and ensure that changes can be included with test updates.

@birtles

This comment has been minimized.

Copy link
Contributor

birtles commented Mar 20, 2018

Looks good. I'll wait for the change to the header before marking it as approved.

Regarding editing the files, as long as I can make (small, careful) hand edits, that sounds fine.

Luke Bjerring
@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 20, 2018

@birtles - updated header. Thanks for taking the time to review!

@foolip

foolip approved these changes Mar 21, 2018

@birtles
Copy link
Contributor

birtles left a comment

Thanks!

@birtles birtles merged commit 0529cde into web-platform-tests:master Mar 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.