Show when animations are running on the compositor thread in the new timeline UI

RESOLVED FIXED in Firefox 44

Status

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

({dev-doc-complete})

unspecified
Firefox 45
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, relnote-firefox 44+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
In bug 1121896, we added an icon to animations that run on the compositor thread.
With bug 1153271, the UI is changing, so we need to port the icon and logic to show it in the new UI too.

Also, more animations are going to run on the compositor now, so it's important that we do this (see http://dbaron.org/log/20150916-compositor-animations ).

The challenge here is to show the icon at the right times. With the previous UI it was simple because the UI was constantly polling the state of animations on the devtools server, so it was easy to know when animations where running on the compositor and when they were not.
The new UI is server-event driven, no polling there.
When the UI receives a new animation for display, it can know its state, but it won't receive events after that telling it when the animation is or isn't running on the compositor.

I think we can assume that it runs there as long as it is playing. When animations are paused, they are taken off the compositor.
(Assignee)

Comment 1

3 years ago
Created attachment 8672307 [details] [diff] [review]
Bug_1205704_-_1_-_Tell_which_animations_are_runnin.diff

Add a label to the tooltip that's displayed when hovering over animations when animations are running on the compositor.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8672308 [details] [diff] [review]
Bug_1205704_-_2_-_Show_an_icon_on_animations_that_.diff

Add an icon on the timeline for animations that run on the compositor.
(Assignee)

Comment 3

3 years ago
Created attachment 8680028 [details]
compositor-thread-icon-tooltip.png

This screenshot shows what the patches add to the UI: an icon to the timeline, and an extra line in the tooltip.
(Assignee)

Comment 4

3 years ago
Created attachment 8680601 [details] [diff] [review]
Bug_1205704_-_Show_an_icon_and_tooltip_when_animat.diff

Cleaned up the patches, folded them together, added a test, and fixed all sorts of icon alignment problems in these edge cases: negative delay, multiple iterations, infinite animations, different playback rate.

You can test with these STR:
- http://jsbin.com/tugikotiyo/edit?css,js,output
- while the animations are running, select the <body> in the jsbin's output iframe
- in the animation inspector you should see 4 animations: 2 css animations and 2 css transitions. 2 of them use margin-left (causes layouts, no compositor thread for this), and 2 others use transform (runs on the compositor).

You should see the icons and the extra info in the tooltip.
Attachment #8672307 - Attachment is obsolete: true
Attachment #8672308 - Attachment is obsolete: true
Attachment #8680601 - Flags: review?(ttromey)

Comment 6

3 years ago
Comment on attachment 8680601 [details] [diff] [review]
Bug_1205704_-_Show_an_icon_and_tooltip_when_animat.diff

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

Looks good, thanks.
Attachment #8680601 - Flags: review?(ttromey) → review+
Any chance this might be uplifted to v44? (Is it v44 we were targeting for the new timeline UI?)

(I'm hoping to plug these animation tools at our Firefox DevCon in Tokyo on Nov 14 and I'd especially like to talk about compositor animations. It would be nice to use v44 if possible.)
(Assignee)

Comment 8

3 years ago
(In reply to Brian Birtles (:birtles) from comment #7)
> Any chance this might be uplifted to v44? (Is it v44 we were targeting for
> the new timeline UI?)
Yes, but a few things didn't make it in time, like this bug.
> (I'm hoping to plug these animation tools at our Firefox DevCon in Tokyo on
> Nov 14 and I'd especially like to talk about compositor animations. It would
> be nice to use v44 if possible.)
I think we can uplift this as it really low risk. I'll first land it to m-c and ask for approval next.

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89a683d15226
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> > (I'm hoping to plug these animation tools at our Firefox DevCon in Tokyo on
> > Nov 14 and I'd especially like to talk about compositor animations. It would
> > be nice to use v44 if possible.)
> 
> I think we can uplift this as it really low risk. I'll first land it to m-c
> and ask for approval next.

Thanks Patrick! That would be great.
(Assignee)

Comment 12

3 years ago
Comment on attachment 8680601 [details] [diff] [review]
Bug_1205704_-_Show_an_icon_and_tooltip_when_animat.diff

Approval Request Comment
[Feature/regressing bug #]: This isn't a regression or a bug, it's a new minor feature that we wanted to ship in 44 but missed the merge date by a day.

[User impact if declined]: If declined, users won't know when animations are running on the compositor thread in devtools. This is quite an important information as it helps people optimize their animations and therefore have less janky websites.
FF44 is a major hit in terms of animation tooling, and it'd be great if this feature made it to 44 too.

[Describe test coverage new/current, TreeHerder]: There are automated browser mochitests running for this, all passed fine on fx-team and are now running on m-c.

[Risks and why]: Given the type of code, the risks are very limited. This really just displays an icon on animations. I don't any chance for regression here.
The only risk is related to how the icon is displayed, there could be bugs around where the icon is displayed, if it's visible correctly. But so far, all tests were successful.

[String/UUID change made/needed]: None
Attachment #8680601 - Flags: approval-mozilla-aurora?
status-b2g-v2.5: fixed → ---
Keywords: dev-doc-needed

Updated

3 years ago
status-firefox44: --- → affected
Comment on attachment 8680601 [details] [diff] [review]
Bug_1205704_-_Show_an_icon_and_tooltip_when_animat.diff

This change has been on Nightly for a few days so feels safe. Let's uplift to Aurora44.
Attachment #8680601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Patrick, is this worth relnoting in 44 release notes? We normally do not update Aurora release notes once published, but I could add this when 44 moves from Aurora to Beta channel. Please let me know.
Flags: needinfo?(pbrosset)
Blocks: 1223204
(Assignee)

Comment 16

3 years ago
(In reply to Ritu Kothari (:ritu) from comment #15)
> Patrick, is this worth relnoting in 44 release notes? We normally do not
> update Aurora release notes once published, but I could add this when 44
> moves from Aurora to Beta channel. Please let me know.
Adding a relnote in Aurora is too late as you said, and there are already several notes for the animation tool there. Plus the compositor icon is not the most important feature anyway, so no problem. Not sure it's worth mentioning when moving to Beta either.

I think we ought to mention this feature a little more once bug 1223204 will be fixed.
Flags: needinfo?(pbrosset)

Comment 17

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/348be28b64b6
status-firefox44: affected → fixed
I've added a section on Firefox 44: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Work_with_animations#Firefox_44

Also see bug 1205681, comment 15 for more on why I've chosen to present it like this.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 20

3 years ago
(In reply to Will Bamberg [:wbamberg] from comment #18)
> I've added a section on Firefox 44:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Work_with_animations#Firefox_44
> 
> Also see bug 1205681, comment 15 for more on why I've chosen to present it
> like this.
See my reply in bug 1205681.
Other than this, the doc you've added for this looks good to me. Thanks!
status-b2g-v2.5: fixed → ---
Flags: needinfo?(pbrosset)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
(Assignee)

Comment 21

3 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: This provides performance insights to users of the animation-inspector in firefoxdevtools
[Suggested wording]: In the animation-inspector timeline, the animations that are running on the compositor thread now have a lightning bolt icon to identify them. This is useful to know when animations are guaranteed to run smoothly.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Added to Fx44 release notes.
relnote-firefox: ? → 44+

Updated

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