1110762 - Add a setCurrentTime method to the AnimationPlayerActor actor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Now that bug 1096044 has landed, we have web animations actors.
Other than returning the list of animation players for a node and allowing consumers to play/pause animations, they're pretty limited at the moment.

The very next thing to add is the ability to control the current time of an animation player.

Comment 1

[Patrick Brosset <:pbro>]

Brian, is there a bug on file already for making currentTime settable on the platform?
The patch you gave me some time ago doesn't apply anymore.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: bug1110762-add-setCurrentTime-to-AnimationPlayerActor v1.patch

Comment 3

[Brian Birtles (:birtles)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #1)
> Brian, is there a bug on file already for making currentTime settable on the
> platform?

Yep, bug 1072037.

> The patch you gave me some time ago doesn't apply anymore.

Ok, I'll upload a new one to bug 1072037.

Comment 4

[Patrick Brosset <:pbro>]

Bug 1072037 isn't marked as fixed yet because it needs a minor extra patch to go first, but the main functionality has landed: animationPlayer.currentTime is now settable!

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8535595 [details] [diff] [review]
bug1110762-add-setCurrentTime-to-AnimationPlayerActor v1.patch

The code has changed quite a bit since that patch was uploaded. I'll upload a new one soon.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1110762/pbrosset

/r/5371 - Bug 1110762 - Add a setCurrentTime method to the animation actor

Pull down this commit:

hg pull review -r 5c95d3ffda22598b159faa23a521c0c03b9f3ffe

Comment 7

[Patrick Brosset <:pbro>]

Hey Panos, this one is a small one, and the people I usually ask to review animation-actor changes have other reviews from me in their queues :)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8577299 [details]
MozReview Request: bz://1110762/pbrosset

https://reviewboard.mozilla.org/r/5369/#review4309

::: toolkit/devtools/server/tests/browser/browser_animation_actors_11.js
(Diff revision 1)
> +  let inspector = InspectorFront(client, form);
> +  let walker = yield inspector.getWalker();
> +  let front = AnimationsFront(client, form);

I would have used "animations" instead of "front" here, but that's OK if you prefer it this way.

::: toolkit/devtools/server/tests/browser/browser_animation_actors_11.js
(Diff revision 1)
> +  ok(state.currentTime >= player.initialState.currentTime,

Wouldn't using > be more correct for this test though? Seeing the same time might indicate the update failed for some reason. Also it's more in line with the next test that seeks backwards and doesn't expect equality.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8577299 [details]
MozReview Request: bz://1110762/pbrosset

https://reviewboard.mozilla.org/r/5369/#review4311

Ship It!

Comment 10

[Patrick Brosset <:pbro>]

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c655db3419
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/32152fbd93eb

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32152fbd93eb

Comment 13

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1110762 - Add a setCurrentTime method to the animation actor