1145246 - Rename Element.getAnimationPlayers to Element.getAnimations

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

The Web Animations spec was recently updated to rename Element.getAnimationPlayers to Element.getAnimations.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Rename Animatable.getAnimationPlayers() to Animatable.getAnimations(), but leave the old name an an alias for the new

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Update Web Animations code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Update layout code for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 5 - Remove the Animatable.getAnimationPlayers() alias for Animatable.getAnimations(). r=birtles

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8580440 [details] [diff] [review]
part 1 - Rename Animatable.getAnimationPlayers() to Animatable.getAnimations(), but leave the old name an an alias for the new

For the webidl change.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8580444 [details] [diff] [review]
part 5 - Remove the Animatable.getAnimationPlayers() alias for Animatable.getAnimations(). r=birtles

For the webidl change.

Comment 8

[Brian Birtles (:birtles)]

I'm about 2/3rds of the way through part 3 but it's taking a while. While I think this would have been better after Q1, I'm grateful you did it. Thanks!

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8580442 [details] [diff] [review]
part 2 - Update Web Animations code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Review of attachment 8580442 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't go through this too deeply but it looks fine. Comments below.

::: dom/animation/test/css-animations/test_animation-player-starttime.html
@@ +194,5 @@
>  // animation. The terms can be found here:
>  //
>  //   http://w3c.github.io/web-animations/#animation-node-phases-and-states
>  //
> +// Note the distinction between "phases start time" and "animation start time".

Is this right? I think it should be "effect start time" and "active interval start time"?

Also, the link should be: http://w3c.github.io/web-animations/#animation-effect-phases-and-states

::: dom/animation/test/css-animations/test_animations-dynamic-changes.html
@@ +61,5 @@
> +  animations = div.getAnimations();
> +  assert_equals(animations[0], animation1,
> +                'First Animation is in same position after update');
> +  assert_equals(animations[1], animation2,
> +                'Second Animation is in same position after update');

We seem to have switched player for Animation (Title Case) but it probably doesn't matter (or maybe that was deliberate?).

::: dom/animation/test/css-transitions/test_animation-player-starttime.html
@@ +187,3 @@
>  //   http://w3c.github.io/web-animations/#animation-node-phases-and-states
>  //
>  // Note the distinction between "player start time" and "animation start time".

We still refer to players here.

::: dom/animation/test/testcommon.js
@@ +43,2 @@
>   *
> + *   Promise.all([animations[0].ready, animations[1].ready, ... animations[N-1].ready]);

Wrap this line.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8580441 [details] [diff] [review]
part 4 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Review of attachment 8580441 [details] [diff] [review]:
-----------------------------------------------------------------

The thing with devtools is we have to support debugging older servers.
Indeed, devtools is split in 2, the client part (the toolbox UI, in Firefox desktop), and the debugger server, potentially running on a remote device or separate firefox process.
This means that you could be using a recent toolbox that expects to retrieve the list of animations from the server via the getAnimationsForNode devtools protocol request, but connected to an older server (running on a b2g device that hasn't been updated instance) which will be expecting getAnimationPlayersForNode instead.
Changing actor names and actor method names is usually a bad idea for this reason. It forces us to have all kinds of nasty 'if (serverSupportsThis)' branches in the client code.

Having said this, we started implementing animation tools early, before the spec was stabilized, so we kind of knew this could happen. And I wouldn't want the devtools code to contain countless references of incorrect names that will just confuse people. So we should change the names as you did, but it's unfortunately not going to be as simple as a search/replace patch. We'll need to test the server capability from the client and probably keep the old AnimationPlayerActor around, even if not used anymore so that the devtools protocol can at least de-serialize responses from older servers that still have it.

Also, there might some be variables with 'player' in the name that we want to keep this way. Like in the UI code, we've got a list players, and I think it's OK to keep calling them players cause that relates more to the widget with the play/pause buttons and timeline than the waapi player object.

Can I suggest that you re-submit a patch that only deals with changing references of el.getAnimationPlayers() to el.getAnimations() in the devtools code, and file another bug to rename devtools actors and methods? I could work on this one.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580440 [details] [diff] [review]
part 1 - Rename Animatable.getAnimationPlayers() to Animatable.getAnimations(), but leave the old name an an alias for the new

I don't know why we want the alias... but I guess the other patch explains..

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580444 [details] [diff] [review]
part 5 - Remove the Animatable.getAnimationPlayers() alias for Animatable.getAnimations(). r=birtles

Mysterious patches :)
I wonder why the part 1 didn't remove the alias.

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 8580440 [details] [diff] [review]
part 1 - Rename Animatable.getAnimationPlayers() to Animatable.getAnimations(), but leave the old name an an alias for the new

https://hg.mozilla.org/integration/mozilla-inbound/rev/0df169bf278a

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8580442 [details] [diff] [review]
part 2 - Update Web Animations code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

https://hg.mozilla.org/integration/mozilla-inbound/rev/c55c5307e557

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8580443 [details] [diff] [review]
part 3 - Update layout code for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

https://hg.mozilla.org/integration/mozilla-inbound/rev/79a60e892217

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8586059 [details] [diff] [review]
part 4 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()

Review of attachment 8586059 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. We'll need to change the devtools actor/method names in a separate bug as discussed. But this is good to go.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0df169bf278a
https://hg.mozilla.org/mozilla-central/rev/c55c5307e557
https://hg.mozilla.org/mozilla-central/rev/79a60e892217
https://hg.mozilla.org/mozilla-central/rev/1778b58593a9
https://hg.mozilla.org/mozilla-central/rev/53ab4a111d5a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f488330059d
https://hg.mozilla.org/mozilla-central/rev/32ee4ace1a31

Comment 20

[Jean-Yves Perrier [:teoli]]

Given that this method is not yet documented and has its own bug with ddn, I only updated:
https://developer.mozilla.org/en-US/Firefox/Releases/40

I let the ddn here if kohei wants to update Site compatibility data.

Comment 21

[Kohei Yoshino]

The Web Animations implementation is still behind the pref so I won't put this on the site compatibility doc.