Jonathan Watt [:jwatt] Assignee
	Description • 9 years ago

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

	Jonathan Watt [:jwatt] Assignee
	Comment 6 • 9 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 7 • 9 years ago

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

For the webidl change.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 8 • 9 years ago

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!

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 9 • 9 years ago

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.

	Patrick Brosset <:pbro>
	Comment 10 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

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..

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 9 years ago

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.

	Jonathan Watt [:jwatt] Assignee
	Comment 13 • 9 years ago

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

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 9 years ago

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

	Jonathan Watt [:jwatt] Assignee
	Comment 15 • 9 years ago

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

	Patrick Brosset <:pbro>
	Comment 17 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 20 • 9 years ago

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.

	Kohei Yoshino [:kohei]
	Comment 21 • 9 years ago

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