Closed Bug 1145246 Opened 5 years ago Closed 5 years ago

Rename Element.getAnimationPlayers to Element.getAnimations

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

The Web Animations spec was recently updated to rename Element.getAnimationPlayers to Element.getAnimations.
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.
Attachment #8580440 - Flags: review?(bugs)
Comment on attachment 8580444 [details] [diff] [review]
part 5 - Remove the Animatable.getAnimationPlayers() alias for Animatable.getAnimations(). r=birtles

For the webidl change.
Attachment #8580444 - Flags: checkin?(bugs)
Attachment #8580440 - Flags: review?(bbirtles) → review+
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 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.
Attachment #8580442 - Flags: review?(bbirtles) → review+
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.
Attachment #8580441 - Flags: review?(pbrosset)
Attachment #8580444 - Flags: checkin?(bugs) → review?(bugs)
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..
Attachment #8580440 - Flags: review?(bugs) → review+
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.
Attachment #8580444 - Flags: review?(bugs) → review+
Attachment #8580443 - Flags: review?(bbirtles) → review+
Attachment #8580444 - Flags: review?(bbirtles) → review+
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
Attachment #8580440 - Flags: checkin+
Attachment #8580441 - Attachment description: part 2 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations() → part 4 - Update DevTools code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()
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
Attachment #8580442 - Attachment description: part 3 - Update Web Animations code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations() → part 2 - Update Web Animations code and tests for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()
Attachment #8580442 - Flags: checkin+
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
Attachment #8580443 - Attachment description: part 4 - Update layout code for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations() → part 3 - Update layout code for the rename of Animatable.getAnimationPlayers() to Animatable.getAnimations()
Attachment #8580443 - Flags: checkin+
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.
Attachment #8586059 - Flags: review?(pbrosset) → review+
Blocks: 1149501
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.
The Web Animations implementation is still behind the pref so I won't put this on the site compatibility doc.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.