1415448 - Tidy up and modernize web-platform-tests for Web Animations

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Brian Birtles (:birtles)]

For a number of bugs blocking bug 1398037 I've been gradually tidying up the associated web-platform-tests. However, rather than doing that test-by-test as I need to change them, it's probably better to go through all the tests and fix up the common issues. That should be easier to review and make the result more consistent.

In this bug I plan to:

* Update the titles of all the tests
* Move/rename a couple of test files that appear to be in the wrong place
* Replace var with const/let as appropriate
* Replace double quotes with single quotes as appropriate
* Use template strings were appropriate
* Use arrow functions

Comment 1

[Brian Birtles (:birtles)]

I started working on this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a60d9d3259e4b525a5eb15cd05881570f698212

I've only fixed the first two items from comment 0 since I encountered so many other problems with the tests I looked at. I might put this first set of patches up for review soon-ish since if there is any review feedback from them, fixing it is likely to bitrot all the other patches in the series badly.

Comment 2

[Brian Birtles (:birtles)]

I'm going to put up the first set of patches for review but there will be a few more (very simply ones) to come.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Update web-platform-tests manifest;

This is simply the result of running:

   ./mach wpt-manifest-update

Review commit: https://reviewboard.mozilla.org/r/198396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198396/

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;

It's not necessary to say "tests" or "tests for" in the title of these tests.
It's obvious from the context that these are test files.

This patch:

* Remove "Tests for" and "tests" from the titles
* Makes the titles match the spec sections they test more closely where
  practical
* Makes the titles more consistent in general (e.g. not putting () after method
  names)
* Updates a few spec links
* Drops a few author annotations since we decided to no longer use them

There are still some inconsistencies:

- The naming of the setTarget.html test file
- The location of the effect-value-visibility.html file
- The getComputedStyle.html test
- etc.

These will be fixed in subsequent patches in this series.

Review commit: https://reviewboard.mozilla.org/r/198398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198398/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Rename setTarget.html to target.html;

This matches better the naming of other test files.

Review commit: https://reviewboard.mozilla.org/r/198400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198400/

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Move the tests for the type of the 'visibility' property to the animation-types section;

Review commit: https://reviewboard.mozilla.org/r/198402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198402/

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Rename discrete-animation.html test file to discrete-type.html;

For consistency with visibility-type.html.

Review commit: https://reviewboard.mozilla.org/r/198404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198404/

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Tidy up test descriptions for AnimationEffectTiming tests;

The test descriptions should generally be testable statements. I'm following the
convention of BDD-style naming where the subject is implied (e.g. 'Has the
default value 0' should be read as '<test subject> has the default value 0').

Review commit: https://reviewboard.mozilla.org/r/198406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198406/

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Drop getComputedStyle.html test in interfaces/AnimationEffectTiming;

This test feels out of place -- there's no getComputedStyle method on
AnimationEffectTiming and we generally test timing properties by calling
getComputedTiming.

This patch moves most of the tests in this file to the corresponding members
whose liveness they check while updating them to use getComputedTiming instead.

The remaining tests that seek the currentTime and check the style with various
values of endDelay should be covered by
timing-model/animation-effects/simple-iteration-progress.html

Review commit: https://reviewboard.mozilla.org/r/198408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198408/

Comment 10

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html;

We plan to merge in tests here from AnimationEffectTiming/getAnimations.html so
in this patch we update the test descriptions first.

Review commit: https://reviewboard.mozilla.org/r/198410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198410/

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html;

As well as moving these tests, this patch simplifies them in several cases and
tries to make them match the existing tests in that file (e.g. using 'animation'
instead of 'anim', using MS_PER_SEC, using assert_array_equals, etc.)

Review commit: https://reviewboard.mozilla.org/r/198412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198412/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927144 [details]
Bug 1415448 - Update web-platform-tests manifest;

https://reviewboard.mozilla.org/r/198396/#review203590

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927145 [details]
Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;

https://reviewboard.mozilla.org/r/198398/#review203598

::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-overlapping-keyframes.html:3
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Effect value computation tests when keyframes overlap</title>
> +<title>The effect value of a keyframe animation effect: Overlapping keyframes</title>

I am curious what the difference between 'keyframe animation effect' and 'keyframe effect' is.  The spec link is 'keyframe-animatioin-effect' but the spec itself says 'keyframe effect'.

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Animation constructor tests</title>
> +<title>Animation constructor</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animation-animation">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Animation constructor tests</title>
> +<title>Animation constructor</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animation-animation">
>  <link rel="author" title="Hiroyuki Ikezoe" href="mailto:[e-mail]">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/duration.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>duration tests</title>
> +<title>AnimationEffectTiming.duration</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-duration">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/endDelay.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>endDelay tests</title>
> +<title>AnimationEffectTiming.endDelay</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-enddelay">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/endDelay.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>endDelay tests</title>
> +<title>AnimationEffectTiming.endDelay</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-enddelay">
>  <link rel="author" title="Ryo Motozawa" href="mailto:[e-mail]">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/getAnimations.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Element.getAnimations tests</title>
> +<title>Element.getAnimations</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">

Nit: https.

I thought the spec link is incorrect but we already have test files for Animatable.getAnimations(), so the title is incorrect?  AnimationEffectTiming?

After I looked subsequent changes, the title seems to be correct, at least it's not incorrect.  I don't think it's really good but it's OK as it is.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/getComputedStyle.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>getComputedStyle tests</title>
> +<title>getComputedStyle</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/iterationStart.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>iterationStart tests</title>
> +<title>AnimationEffectTiming.iterationStart</title>
>  <link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttiming-iterationstart">
>  <link rel="author" title="Daisuke Akatsuka" href="mailto:[e-mail]">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/getComputedTiming.html:3
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>KeyframeEffectReadOnly getComputedTiming() tests</title>
> +<title>KeyframeEffectReadOnly.getComputedTiming</title>

This should be AnimationEffectReadOnly.getComputedTiming?  I am not sure though.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:4
(Diff revision 1)
>  <!doctype html>
>  <meta charset=utf-8>
> -<title>KeyframeEffect.iterationComposite tests</title>
> +<title>KeyframeEffect.iterationComposite</title>
>  <link rel="help" href="https://w3c.github.io/web-animations/#effect-accumulation-section">

https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-iterationcomposite ?

Oh no, this file tests accumulate only, so the title should be ' Effect accumulation'?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927146 [details]
Bug 1415448 - Rename setTarget.html to target.html;

https://reviewboard.mozilla.org/r/198400/#review203606

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927147 [details]
Bug 1415448 - Move the tests for the type of the 'visibility' property to the animation-types section;

https://reviewboard.mozilla.org/r/198402/#review203608

Should we drop -type in the file name?  Though I don't have strong opnition, it's up to you.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927148 [details]
Bug 1415448 - Rename discrete-animation.html test file to discrete-type.html;

https://reviewboard.mozilla.org/r/198404/#review203612

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927149 [details]
Bug 1415448 - Tidy up test descriptions for AnimationEffectTiming tests;

https://reviewboard.mozilla.org/r/198406/#review203616

It would be nice to modify descriptions in README.md as well?

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/duration.html:59
(Diff revision 1)
>  test(function(t) {
>    var div = createDiv(t);
>    assert_throws({ name: 'TypeError' }, function() {
>      div.animate({ opacity: [ 0, 1 ] }, -1);
>    });
> -}, 'set negative duration in animate using a duration parameter');
> +}, 'animate() throws when passed a negative number');

We might want to move these tests that throw on animate() method into other files in future.

Comment 18

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> duration.html:59
> (Diff revision 1)
> >  test(function(t) {
> >    var div = createDiv(t);
> >    assert_throws({ name: 'TypeError' }, function() {
> >      div.animate({ opacity: [ 0, 1 ] }, -1);
> >    });
> > -}, 'set negative duration in animate using a duration parameter');
> > +}, 'animate() throws when passed a negative number');
> 
> We might want to move these tests that throw on animate() method into other
> files in future.

Yes, definitely. There are so many things that need fixing in these files. This bug is definitely not going to fix everything but hopefully it fixes a lot of common things. We can fix more specific problems when we next touch these files.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927150 [details]
Bug 1415448 - Drop getComputedStyle.html test in interfaces/AnimationEffectTiming;

https://reviewboard.mozilla.org/r/198408/#review203620

MozReview is really useless for this kind of changes.  I haven't checked tests were moved correctly, but I believe it did.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927151 [details]
Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html;

https://reviewboard.mozilla.org/r/198410/#review203622

::: commit-message-0ee73:3
(Diff revision 1)
> +Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html; r?hiro
> +
> +We plan to merge in tests here from AnimationEffectTiming/getAnimations.html so

Nice!

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8927152 [details]
Bug 1415448 - Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html;

https://reviewboard.mozilla.org/r/198412/#review203624

r=me with dropping AnimationEffectTiming/getAnimations.html.

::: testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html:85
(Diff revision 1)
> +                      'Animation should be returned after extending the'
> +                      + ' duration');
> +
> +  animation.effect.timing.duration = 0;
> +  assert_array_equals(div.getAnimations(), [],
> +                      'Animations should not be returned after setting the'

Nit: Animaion instead of Animations?

Comment 22

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8927145 [details]
> Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;
> 
> https://reviewboard.mozilla.org/r/198398/#review203598
> 
> :::
> testing/web-platform/tests/web-animations/animation-model/keyframe-effects/
> effect-value-overlapping-keyframes.html:3
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>Effect value computation tests when keyframes overlap</title>
> > +<title>The effect value of a keyframe animation effect: Overlapping keyframes</title>
> 
> I am curious what the difference between 'keyframe animation effect' and
> 'keyframe effect' is.  The spec link is 'keyframe-animatioin-effect' but the
> spec itself says 'keyframe effect'.

Yeah, you're right. Keyframe effect is sufficient here.

> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> getAnimations.html:4
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>Element.getAnimations tests</title>
> > +<title>Element.getAnimations</title>
> >  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">
> 
> Nit: https.
> 
> I thought the spec link is incorrect but we already have test files for
> Animatable.getAnimations(), so the title is incorrect? 
> AnimationEffectTiming?
> 
> After I looked subsequent changes, the title seems to be correct, at least
> it's not incorrect.  I don't think it's really good but it's OK as it is.

Yes, later I will delete this file altogether so for now I will not change the spec link.

> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> getComputedStyle.html:4
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>getComputedStyle tests</title>
> > +<title>getComputedStyle</title>
> >  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">
> 
> Nit: https.

Again, I am going to delete this file so I will leave the spec link as-is (so it doesn't bitrot the patch where I delete the file).

> :::
> testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/
> getComputedTiming.html:3
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>KeyframeEffectReadOnly getComputedTiming() tests</title>
> > +<title>KeyframeEffectReadOnly.getComputedTiming</title>
> 
> This should be AnimationEffectReadOnly.getComputedTiming?  I am not sure
> though.

Yes, probably. I've made a note to do that in a follow-up.

> :::
> testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/
> iterationComposite.html:4
> (Diff revision 1)
> >  <!doctype html>
> >  <meta charset=utf-8>
> > -<title>KeyframeEffect.iterationComposite tests</title>
> > +<title>KeyframeEffect.iterationComposite</title>
> >  <link rel="help" href="https://w3c.github.io/web-animations/#effect-accumulation-section">
> 
> https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-
> iterationcomposite ?
> 
> Oh no, this file tests accumulate only, so the title should be ' Effect
> accumulation'?

Yeah, this test should be moved to the animation-model. I've made a note to do that too.

Comment 23

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Update web-platform-tests manifest;

This is simply the result of running:

   ./mach wpt-manifest-update

Review commit: https://reviewboard.mozilla.org/r/198512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198512/

Comment 24

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;

It's not necessary to say "tests" or "tests for" in the title of these tests.
It's obvious from the context that these are test files.

This patch:

* Remove "Tests for" and "tests" from the titles
* Makes the titles match the spec sections they test more closely where
  practical
* Makes the titles more consistent in general (e.g. not putting () after method
  names)
* Updates a few spec links
* Drops a few author annotations since we decided to no longer use them

There are still some inconsistencies:

- The naming of the setTarget.html test file
- The location of the effect-value-visibility.html file
- The getComputedStyle.html test
- etc.

These will be fixed in subsequent patches in this series.

Review commit: https://reviewboard.mozilla.org/r/198514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198514/

Comment 25

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Rename setTarget.html to target.html;

This matches better the naming of other test files.

Review commit: https://reviewboard.mozilla.org/r/198516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198516/

Comment 26

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Move the tests for the type of the 'visibility' property to the animation-types section;

Review commit: https://reviewboard.mozilla.org/r/198518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198518/

Comment 27

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Rename discrete-animation.html test file to discrete.html;

For consistency with visibility.html.

Review commit: https://reviewboard.mozilla.org/r/198520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198520/

Comment 28

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Tidy up test descriptions for AnimationEffectTiming tests;

The test descriptions should generally be testable statements. I'm following the
convention of BDD-style naming where the subject is implied (e.g. 'Has the
default value 0' should be read as '<test subject> has the default value 0').

Review commit: https://reviewboard.mozilla.org/r/198522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198522/

Comment 29

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Drop getComputedStyle.html test in interfaces/AnimationEffectTiming;

This test feels out of place -- there's no getComputedStyle method on
AnimationEffectTiming and we generally test timing properties by calling
getComputedTiming.

This patch moves most of the tests in this file to the corresponding members
whose liveness they check while updating them to use getComputedTiming instead.

The remaining tests that seek the currentTime and check the style with various
values of endDelay should be covered by
timing-model/animation-effects/simple-iteration-progress.html

Review commit: https://reviewboard.mozilla.org/r/198524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198524/

Comment 30

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html;

We plan to merge in tests here from AnimationEffectTiming/getAnimations.html so
in this patch we update the test descriptions first.

Review commit: https://reviewboard.mozilla.org/r/198526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198526/

Comment 31

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html;

As well as moving these tests, this patch simplifies them in several cases and
tries to make them match the existing tests in that file (e.g. using 'animation'
instead of 'anim', using MS_PER_SEC, using assert_array_equals, etc.)

Review commit: https://reviewboard.mozilla.org/r/198528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198528/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab43c3e25edd
Update web-platform-tests manifest; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b0fcc3f732
Tidy up titles of Web Animations web-platform-tests; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/316d85f7d5dc
Rename setTarget.html to target.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9a5a6c1b06
Move the tests for the type of the 'visibility' property to the animation-types section; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dee378cbad
Rename discrete-animation.html test file to discrete.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42ad6fd6afa
Tidy up test descriptions for AnimationEffectTiming tests; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0cbe23c132e
Drop getComputedStyle.html test in interfaces/AnimationEffectTiming; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/edba74152c40
Update test descriptions in interfaces/Animatable/getAnimations.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b652edba2199
Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html; r=hiro

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab43c3e25edd
https://hg.mozilla.org/mozilla-central/rev/c6b0fcc3f732
https://hg.mozilla.org/mozilla-central/rev/316d85f7d5dc
https://hg.mozilla.org/mozilla-central/rev/8a9a5a6c1b06
https://hg.mozilla.org/mozilla-central/rev/e3dee378cbad
https://hg.mozilla.org/mozilla-central/rev/a42ad6fd6afa
https://hg.mozilla.org/mozilla-central/rev/b0cbe23c132e
https://hg.mozilla.org/mozilla-central/rev/edba74152c40
https://hg.mozilla.org/mozilla-central/rev/b652edba2199

Comment 34

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Use arrow functions in web-platform-tests/web-animations;

Review commit: https://reviewboard.mozilla.org/r/200090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200090/

Comment 35

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Replace var with const/let in web-platform-tests/web-animations;

Because 'const' is longer than 'var', in quite a number of places this patch
also updates the indentation of subsequent lines.

Also, in a number of cases this means the line now needs to be wrapped. In
general I've used 'prettier' to do the line wrapping which means that the line
wrapping style differs a little from the existing code (which is already quite
inconsistent). Hopefully in the future we can use prettier on all files to make
this more consistent.

Also, in some cases, to avoid further line wrapping, this patch uses shorthand
property names (specifically replacing 'composite: composite' with just
'composite'). This appears to be supported in all browsers like to implement Web
Animations (Firefox 33+, Chrome 47+, Edge, Safari 9+).

Review commit: https://reviewboard.mozilla.org/r/200092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200092/

Comment 36

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Consistently use single quotes for string literals in web-platform-tests/web-animations;

We will introduce template literals in the next patch in this series.

Review commit: https://reviewboard.mozilla.org/r/200094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200094/

Comment 37

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Use template literals in a few places in web-platform-tests/web-animations;

This patch does not attempt to be exhaustive but fixes a few instances where to
are obvious benefits to using template literals.

Review commit: https://reviewboard.mozilla.org/r/200096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200096/

Comment 38

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Consistently use strict mode in web-platform-tests/web-animations;

Review commit: https://reviewboard.mozilla.org/r/200098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200098/

Comment 39

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Use for...of instead of forEach in web-platform-tests/web-animations;

Review commit: https://reviewboard.mozilla.org/r/200100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200100/

Comment 40

[Brian Birtles (:birtles)]

Attachment: Bug 1415448 - Update web-platform-tests MANIFEST.json;

This is just the result of running:

  ./mach wpt-manifest-update

Review commit: https://reviewboard.mozilla.org/r/200102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200102/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928809 [details]
Bug 1415448 - Use arrow functions in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200090/#review205224

It would be nice to update README.md and testcommon.js as well.  IIUC, below two files also needs to be updated.

animation-model/keyframe-effects/effect-value-transformed-distance.html
animation-model/animation-types/property-types.js

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928810 [details]
Bug 1415448 - Replace var with const/let in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200092/#review205230

Looks good. I did check only 'let's though.

::: testing/web-platform/tests/web-animations/interfaces/Animation/ready.html:37
(Diff revision 1)
> -  var div = createDiv(t);
> -  var animation = div.animate(null, 100 * MS_PER_SEC);
> +  let div = createDiv(t);
> +  let animation = div.animate(null, 100 * MS_PER_SEC);

Nit: const.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928811 [details]
Bug 1415448 - Consistently use single quotes for string literals in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200094/#review205232

I guess IDL part in idlharness.html intentionally uses double quote there.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928812 [details]
Bug 1415448 - Use template literals in a few places in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200096/#review205238

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928813 [details]
Bug 1415448 - Consistently use strict mode in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200098/#review205242

Below also need to be updated.

animation-model/keyframe-effects/effect-value-context.html
animation-model/animation-types/property-types.js
resources/easing-tests.js
resources/effect-tests.js

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928815 [details]
Bug 1415448 - Update web-platform-tests MANIFEST.json;

https://reviewboard.mozilla.org/r/200102/#review205248

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8928814 [details]
Bug 1415448 - Use for...of instead of forEach in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200100/#review205250

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> Comment on attachment 8928813 [details]
> Bug 1415448 - Consistently use strict mode in
> web-platform-tests/web-animations;
> 
> https://reviewboard.mozilla.org/r/200098/#review205242
> 
> Below also need to be updated.
> 
> animation-model/keyframe-effects/effect-value-context.html
> animation-model/animation-types/property-types.js
> resources/easing-tests.js
> resources/effect-tests.js

Do'h!  I did grep without applying the patch.  Never mind about this comment.

Comment 49

[Brian Birtles (:birtles)]

Comment on attachment 8928809 [details]
Bug 1415448 - Use arrow functions in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200090/diff/1-2/

Comment 50

[Brian Birtles (:birtles)]

Comment on attachment 8928810 [details]
Bug 1415448 - Replace var with const/let in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200092/diff/1-2/

Comment 51

[Brian Birtles (:birtles)]

Comment on attachment 8928811 [details]
Bug 1415448 - Consistently use single quotes for string literals in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200094/diff/1-2/

Comment 52

[Brian Birtles (:birtles)]

Comment on attachment 8928812 [details]
Bug 1415448 - Use template literals in a few places in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200096/diff/1-2/

Comment 53

[Brian Birtles (:birtles)]

Comment on attachment 8928813 [details]
Bug 1415448 - Consistently use strict mode in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200098/diff/1-2/

Comment 54

[Brian Birtles (:birtles)]

Comment on attachment 8928814 [details]
Bug 1415448 - Use for...of instead of forEach in web-platform-tests/web-animations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200100/diff/1-2/

Comment 55

[Brian Birtles (:birtles)]

Comment on attachment 8928815 [details]
Bug 1415448 - Update web-platform-tests MANIFEST.json;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200102/diff/1-2/

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff031af585d8
Use arrow functions in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/31f60780e3c0
Replace var with const/let in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/b0811e58e932
Consistently use single quotes for string literals in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/066264c8cc1f
Use template literals in a few places in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/355ad237707a
Consistently use strict mode in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5d398f92e237
Use for...of instead of forEach in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/1735548b5e14
Update web-platform-tests MANIFEST.json; r=hiro

Comment 57

[Brian Birtles (:birtles)]

All that remains here now is:

* Move interfaces/KeyframeEffect/getComputedTiming.html to AnimationEffectTiming
* Move KeyframeEffect/iterationComposite.html to animation-model

Comment 58

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #57)
> All that remains here now is:
> 
> * Move interfaces/KeyframeEffect/getComputedTiming.html to
> AnimationEffectTiming
> * Move KeyframeEffect/iterationComposite.html to animation-model

Splitting off bug 1417808 for this because MozReview.

Comment 59

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ff031af585d8
https://hg.mozilla.org/mozilla-central/rev/31f60780e3c0
https://hg.mozilla.org/mozilla-central/rev/b0811e58e932
https://hg.mozilla.org/mozilla-central/rev/066264c8cc1f
https://hg.mozilla.org/mozilla-central/rev/355ad237707a
https://hg.mozilla.org/mozilla-central/rev/5d398f92e237
https://hg.mozilla.org/mozilla-central/rev/1735548b5e14