All users were logged out of Bugzilla on October 13th, 2018

Devtools AnimationPlayerActor should be aware of CSS animations start delay

RESOLVED FIXED in Firefox 37

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 37
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 927349
(Assignee)

Comment 1

4 years ago
Created attachment 8538461 [details] [diff] [review]
bug1113091-make-animation-actor-aware-of-ready-promise v1.patch
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8538472 [details] [diff] [review]
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.
Attachment #8538461 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8538473 [details] [diff] [review]
bug1113091-make-animation-actor-aware-of-ready-promise v3.patch
Attachment #8538472 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 985861
(Assignee)

Updated

4 years ago
Blocks: 1105825
(Assignee)

Comment 5

4 years ago
Created attachment 8545276 [details] [diff] [review]
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.
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+
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(Assignee)

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.