1113091 - Devtools AnimationPlayerActor should be aware of CSS animations start delay

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

With bug 927349, when CSS animations start, they first go through a short pending state before being run.
As part of this change, AnimationPlayer objects have a |ready| property, which is a promise that resolves when the animation actually starts.

This promise is also recreated and resolved when the animation is paused and played again.

The AnimationPlayerActor devtools actor introduced in bug 1096044 should be aware of this mechanism and also expose the promise on the corresponding Front object.

This would help consumers (especially tests) wait for the effective start of an animation, in case that consumer just created it just before.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1113091-make-animation-actor-aware-of-ready-promise v1.patch

Comment 2

[Patrick Brosset <:pbro>]

It turns out the approach I chose in attachment 8538461 [details] [diff] [review] isn't working, because protocol events are only sent once at least one request has been made. So in the case where the animation has already started, the ready event is never sent.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug1113091-make-animation-actor-aware-of-ready-promise v2.patch

This approach works, and is simpler. I'm just exposing the ready promise as an actor method. So, consumers will be able to use it just as they would the normal ready promise on the player object.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1113091-make-animation-actor-aware-of-ready-promise v3.patch

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1113091-make-animation-actor-aware-of-ready-promise v4.patch

Rebased. This should be ready to land, but I'd rather way until bug 1112422 is resolved (to make the waapi available to chrome code) and then bug 1114780 (to re-enable the actor tests) before I land this.

Brian: could you review this simple change?
The goal is to expose the animation player ready promise on the AnimationPlayerActor.
This promise was very recently introduced (this week) in the API and it's needed to make one of the actor tests pass.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8545276 [details] [diff] [review]
bug1113091-make-animation-actor-aware-of-ready-promise v4.patch

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

Do we have any older servers relying on this actor?  If so, we should be aware that the ready function won't be available for them - doesn't matter for this test, but would matter for changes in the future using `ready`, and it may be hard to track since it would be outside of this bug.  We could maybe use the actorHasMethod function when we need to use it, instead of adding a trait: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/target.js#261

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8545276 [details] [diff] [review]
> bug1113091-make-animation-actor-aware-of-ready-promise v4.patch
> 
> Review of attachment 8545276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have any older servers relying on this actor?  If so, we should be
> aware that the ready function won't be available for them - doesn't matter
> for this test, but would matter for changes in the future using `ready`, and
> it may be hard to track since it would be outside of this bug.  We could
> maybe use the actorHasMethod function when we need to use it, instead of
> adding a trait:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> target.js#261
No we don't need to worry about backward compatibility right now, the actor hasn't been released yet.

Comment 8

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a040fe5b1fe1

Comment 9

[Patrick Brosset <:pbro>]

This try build shows docshell leaks that don't seem related to this change, and I don't see why this change would cause any sort of leaks. It just adds a new actor method that isn't even used yet (since the test is still disabled).
Hopefully this new try build should be better: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8f2d7eb92248

Comment 10

[Patrick Brosset <:pbro>]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/2684f09cf985

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2684f09cf985