Closed Bug 1113091 Opened 9 years ago Closed 9 years ago

Devtools AnimationPlayerActor should be aware of CSS animations start delay

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 927349
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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.
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.
Attachment #8538461 - Attachment is obsolete: true
Attachment #8538472 - Attachment is obsolete: true
Blocks: 985861
Blocks: 1105825
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.
Attachment #8538473 - Attachment is obsolete: true
Attachment #8545276 - Flags: review?(bgrinstead)
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
Attachment #8545276 - Flags: review?(bgrinstead) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/2684f09cf985
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: