Closed Bug 1121896 Opened 5 years ago Closed 5 years ago

Show users when an animation is running on the compositor in the Animations panel

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: pbro, Assigned: edoput, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 9 obsolete files)

The Web Animations API provides us with the isRunningOnCompositor property on AnimationPlayer objects.
Knowing whether an animation is running on the compositor or not is useful for animation authors to know whether they are hitting the fast path or not.

It would therefore be useful to highlight those animations that are currently running on the compositor in the animation panel UI.
I think just an icon with a tooltip would be sufficient.

Note that this is currently only true for Firefox OS though (where we have compositor animations enabled), and will be false whenever the animation is paused as it is taken off the compositor then.
Blocks: 1120900
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #0)
> Note that this is currently only true for Firefox OS though (where we have
> compositor animations enabled), and will be false whenever the animation is
> paused as it is taken off the compositor then.
David Baron recently landed a patch in bug 980770 that enables off main thread animations on desktop too (only for opacity and transform for now). So that makes this bug more interesting.
Mentor: pbrosset
Whiteboard: [good first bug][lang=js]
Hi Patrick,

I can work on this if you could guide me where I need to make the changes.
Flags: needinfo?(pbrosset)
Thanks. I've assigned the bug to you.

The file responsible for displaying the UI widget for a given animation is:
/browser/devtools/animationinspector/animation-panel.js

And in that file, the class that generates/updates the markup is:
PlayerWidget

In that class, the information that the corresponding animation is running on the compositor is already accessible via: this.player.state.isRunningOnCompositor.

Know that the player's state is updated on a regular basis so this boolean can be updated dynamically.
onStateChanged is the callback that runs every time the state is updated.

In terms of UI, to be honest, I haven't thought a lot about how we could represent this piece of information yet. So let's start simple for now and have a small icon indicator in the meta-data header part of the PlayerWidget (that's where the animation name, duration, etc. are displayed).

This part is actually managed by another widget: PlayerMetaDataHeader which gets refreshed by PlayerWidget whenever the state changes.
The PlayerMetaDataHeader's init function is where the markup is created the first time, and then the render(state) method is called whenever the state changes.
So you should be able to quite easily add an indicator in there that shows the user if the animation is running on the compositor or not.
Assignee: nobody → chiragbhatia2006
Flags: needinfo?(pbrosset)
Patrick,

from what I understand, I should be adding the code below this line in the "running" case:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#427

I'm guessing I need to check the value of bool this.player.state.isRunningOnCompositor, and if true, I should create an object of the class PlayerWidget. But I didn't understand how to go ahead from here.

Can you please give me some more details on how to go forward with this?
Flags: needinfo?(pbrosset)
(In reply to Chirag Bhatia from comment #4)
> Patrick,
> 
> from what I understand, I should be adding the code below this line in the
> "running" case:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> animationinspector/animation-panel.js#427
I think this new indicator should rather be in the PlayerMetaDataHeader: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#577
So, you should create a new piece of UI for it in the init function  here: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#584
And then update it dynamically, when the state changes here:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#673
by using something like this:
if (state.isRunningOnCompositor !== this.state.isRunningOnCompositor) {
  // ... update the UI
}
Flags: needinfo?(pbrosset)
Forgot to mark as assigned.
Chirag, when do you think you could have a fix ready for this?
Status: NEW → ASSIGNED
Flags: needinfo?(chiragbhatia2006)
Hi Patrick,

Sorry for the late reply. I've not been able to put aside time for this lately.

I think it would be better to assign this bug to someone else. Thanks for your help though, I really appreciate it.
Flags: needinfo?(chiragbhatia2006)
Assignee: chiragbhatia2006 → nobody
Status: ASSIGNED → NEW
Attached patch devtool.show.compositor (obsolete) — Splinter Review
draft patch
Hello Patrick,

I attached a patch that still need polishing but I think that it should do the hard work.

Edoardo
Attached image Schermata da 2015-05-28 23:09:25.png (obsolete) —
a screenshot showing the new element with the tooltip
Just a suggestion- instead of hardcoding the string of the Icon name in components.js you may add the string value to https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/animationinspector.properties and set it in your code as

this.compositorIcon.textContent = L10N.getStr("<KEY_VALUE>");

-for localization support.
Attached patch devtool.show.compositor (obsolete) — Splinter Review
As requested, the updated patch with localization draft. Thanks for the suggestion.
Attachment #8612537 - Attachment is obsolete: true
Attachment #8613246 - Flags: review?(pbrosset)
Attachment #8613246 - Flags: review?(avikpal.me)
Comment on attachment 8613246 [details] [diff] [review]
devtool.show.compositor

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

This is definitely the right direction, the code is nice and looks exactly like what we need.
Now, we need some css styling to make it look nice:
- perhaps have the icon to the left of "duration" instead of aligned to the far right, so that the duration, delay, iterations are always right-aligned,
- we'll need to find an image icon instead of the button, something that conveys the idea that the animation is on the "fast track" (all devtools images are in /browser/themes/shared/devtools/images/).
Next, we'll also need to add a new test for this, you should look into the existing tests at /browser/devtools/animationinspector/test/ , there is already a number of them that check that the duration, delay, iteration information are displayed correctly.
Attachment #8613246 - Flags: review?(pbrosset) → feedback+
Attached image rocket.png (obsolete) —
icon proposal
I could not find a suitable icon so I thought that I could give a try
I am a little confused by the way to style the element, I found the animationinspector.css file but when I tried to reference a new icon in the images directory I got a "element not found" in the computed properties.

Moreover what should we add this node to? To the metadata node or to the "root" node of the PlayerMetaDataHeader?
(In reply to Edoardo Putti [:edoput] from comment #16)
> I am a little confused by the way to style the element, I found the
> animationinspector.css file but when I tried to reference a new icon in the
> images directory I got a "element not found" in the computed properties.
So, if you add a new image to devtools, you also need to reference it in jar.mn files so they get packaged at build time.
If you put your image in /browser/themes/shared/devtools/images/, then reference that path in:
/browser/themes/windows/jar.mn
/browser/themes/osx/jar.mn
/browser/themes/linux/jar.mn
Search for other devtools images in these files, it will be obvious what you need to add.

> Moreover what should we add this node to? To the metadata node or to the
> "root" node of the PlayerMetaDataHeader?
The node should be appended to metaData in PlayerMetaDataHeader I think, just like the duration label.
Attached patch devtool.show.compositor (obsolete) — Splinter Review
Another try,

The icon is now a "sort of" lightning, the rocket was too blurry as an icon.
The icon is now referenced in the 3 platform files.
The css to show the icon is just a quick fix, maybe the icon should be scaled to 12px or something.
I moved the new metadata to the top of the metadata-header element, it shouldn't be a problem in the automated test.
I've added a very dummy test, is it enough?
Attachment #8613246 - Attachment is obsolete: true
Attachment #8613523 - Attachment is obsolete: true
Attachment #8613246 - Flags: review?(avikpal.me)
screenshot
Attachment #8612539 - Attachment is obsolete: true
Attached patch devtool.show.compositor (obsolete) — Splinter Review
The test is working now, I got tricked by the test command. It didn't execute anything before so I could not provide a working test, sorry.
Attachment #8615933 - Attachment is obsolete: true
Comment on attachment 8615985 [details] [diff] [review]
devtool.show.compositor

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

This is very nice, thanks.
I've made a number of comments below. The main problem I found was that the tooltip is really hard to get to. For me it only appears if I hover over the top 1px of the image. This is because of the <input /> that we use for the timeline, it sticks out of its container somehow (for a reason I was not able to explain yet) and prevents the mouse from hovering nearby elements.
I'd like to fix the underlying issue, but I can't figure out how, so instead, you can simply fix this by adding this to the CSS:

.animation-title .meta-data .compositor-icon {
  z-index: 1;
  position: relative;
}

::: browser/devtools/animationinspector/components.js
@@ +76,5 @@
> +    this.compositorIcon = createNode({
> +       parent: metaData,
> +       nodeType: "span",
> +       attributes: {
> +         "style": "display:none",

Can you remove this inline style and instead add display:none to the css file?

::: browser/devtools/animationinspector/test/browser_animation_playerWidgets_compositor_icon.js
@@ +16,5 @@
> +  let compositorEl= panel.playerWidgets[0].el.querySelector(".compositor-icon");
> +  ok(compositorEl, "The compositor-icon element exists");
> +
> +  ok(isNodeVisible(compositorEl),
> +    "The compositor icon is visible, since the animation is running on compositor thread");

I'm worried that this test may sometimes fail on our test infra, depending on the machine that runs the test.
Let's keep this in for the time being, and we'll see on TRY if that fails. If it does, I suggest we change this test to temper the AnimationPlayer at run-time and assert that the icon is visible.

@@ +17,5 @@
> +  ok(compositorEl, "The compositor-icon element exists");
> +
> +  ok(isNodeVisible(compositorEl),
> +    "The compositor icon is visible, since the animation is running on compositor thread");
> +

nit: remove this empty line.

::: browser/themes/osx/jar.mn
@@ +540,5 @@
>    skin/classic/browser/devtools/app-manager/plus.svg                  (../shared/devtools/app-manager/images/plus.svg)
>    skin/classic/browser/devtools/app-manager/remove.svg                (../shared/devtools/app-manager/images/remove.svg)
>    skin/classic/browser/devtools/app-manager/add.svg                   (../shared/devtools/app-manager/images/add.svg)
>    skin/classic/browser/devtools/app-manager/index-icons.svg           (../shared/devtools/app-manager/images/index-icons.svg)
> +  skin/classic/browser/devtools/rocket.svg                            (../shared/devtools/images/rocket.svg)

This is a leftover change that should be removed. Especially that it breaks the build for me locally since rocket.svg does not exist.

::: browser/themes/shared/devtools/animationinspector.css
@@ +147,5 @@
>    margin: 0 .5em;
>  }
>  
> +.animation-title .meta-data .compositor-icon::before {
> +  content: url(animation-fast-track.svg);

This content pushes the line-height a little bit, so when an animation stops being run on the compositor (if you pause it for instance), then the header jumps a bit.
I would suggest doing this instead:

content: "";
background-image: url(animation-fast-track.svg);
padding-left: 12px;

Also, can you make the image be a little bit smaller so it's roughly the same size as the text.

@@ +149,5 @@
>  
> +.animation-title .meta-data .compositor-icon::before {
> +  content: url(animation-fast-track.svg);
> +  height: 80%;
> +  width: 80%;

These don't make any difference for me locally, can you remove the width/height?

::: browser/themes/shared/devtools/images/animation-fast-track.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<!-- Created with Inkscape (http://www.inkscape.org/) -->

This comment should be removed, and instead, you should add the license header like in the other svg files in the same directory:

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Also, the file contains a lot of tags and attributes that aren't needed for the image to be displayed properly, a lot of inkspace metadata that should be removed.
Attached patch devtool.show.compositor (obsolete) — Splinter Review
Candidate with:

- dummy test
- "clean" svg
- interface working
- css style referencing element instead of ::before
Attachment #8615985 - Attachment is obsolete: true
Attachment #8616194 - Flags: review?(pbrosset)
Comment on attachment 8616194 [details] [diff] [review]
devtool.show.compositor

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

This looks great, thanks for addressing my comments.
I only have minor comments about code formatting (see https://wiki.mozilla.org/DevTools/CodingStandards) but nothing that requires a new review.
Also, can you please update the commit message? It should be formatted like this:
Bug <number> - <short description>; r=<reviewername>
So in your case, something like:
Bug 1121896 - Show when animations are running on the compositor in the animation panel; r=pbrosset
Once done, please upload the new patch here and mark it as R+
We need to push this to TRY to make sure all tests still pass by TRY is closed right now, I'll do this later.

::: browser/devtools/animationinspector/components.js
@@ +78,5 @@
> +       nodeType: "span",
> +       attributes: {
> +         "class": "compositor-icon"
> +       }
> +     });

nit: the indentation is off on the previous block, the line comment is indented by 5 spaces instead of 4, the option properties are indented by 7 instead of 6, etc...
Can you clean this up?

@@ +197,5 @@
> +       } else {
> +         // Hide the compositor icon
> +         this.compositorIcon.style.display = "none";
> +       }
> +     }

nit: indentation is also off by 1 space here.
For info, we started using eslint on the devtools code base to help with these and avoid having to look at that during reviews, feel free to take a look at this doc: https://wiki.mozilla.org/DevTools/CodingStandards

::: browser/devtools/animationinspector/test/browser_animation_playerWidgets_compositor_icon.js
@@ +16,5 @@
> +  let compositorEl= panel.playerWidgets[0].el.querySelector(".compositor-icon");
> +  ok(compositorEl, "The compositor-icon element exists");
> +  ok(isNodeVisible(compositorEl),
> +    "The compositor icon is visible, since the animation is running on compositor thread");
> +

nit: please remove this extra empty line.

::: browser/themes/shared/devtools/animationinspector.css
@@ +150,5 @@
> +.animation-title .meta-data .compositor-icon {
> +    display: none;
> +    background-image: url("animation-fast-track.svg");
> +    background-repeat: no-repeat;
> +    z-index: 1;

I think the z-index and position trick requires a comment because it's not immediately visible why it's useful, even removing it from the code won't change anything visually, so it's very probable that it gets removed by mistake in the future.
So can you just add a comment like:

/* Make sure the icon is positioned above the timeline range input
   so that its tooltip appears on hover */
Attachment #8616194 - Flags: review?(pbrosset) → review+
Reminder to push to TRY when it opens again.
Flags: needinfo?(pbrosset)
Pending try push for the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4834d9e363ef
Please keep an eye on this page, once all test jobs turns green we can ask for the patch to be checked-in. Otherwise, that means the patch causes some test failures that we'll need to investigate.

Please note that the current patch does not apply cleanly on fx-team anymore, it needs to be rebased. Can you make sure you update to the latest fx-team and rebase before submitting a new patch with the corrections from comment 23.
Flags: needinfo?(pbrosset)
> Please note that the current patch does not apply cleanly on fx-team anymore, it needs to be rebased. Can you make sure you update to the latest fx-team and rebase before submitting a new patch with the corrections from comment 23.

I think I made a mess. I rebased the patch this morning but I also commited the patch in the repo history. I surely don't have push access to the remote repository, so what do I do next?

This is the current history in my local repo.

>changeset:   248777:57e668dfdea3
>tag:         tip
>utente:      Edoardo Putti <edoardo.putti@gmail.com>
>data:        Sun Jun 14 11:01:22 2015 +0200
>sommario:    Bug 1121896 - Show when animations are running on the compositor in the animation panel; >r=pbrosset
>
>changeset:   248776:c223b8844264
>genitore:    248720:e3ab171ddd37
>genitore:    248775:f9e71f980245
>utente:      Phil Ringnalda <philringnalda@gmail.com>
>data:        Sat Jun 13 19:49:13 2015 -0700
>sommario:    Merge m-i to m-c, a=merge
>
>changeset:   248775:f9e71f980245
>utente:      Phil Ringnalda <philringnalda@gmail.com>
>data:        Sat Jun 13 14:41:15 2015 -0700
>sommario:    Back out fd36716d1f9d (bug 1162986) for mostly-Win8-debug devtools crashes
(In reply to Edoardo Putti [:edoput] from comment #26)
> > Please note that the current patch does not apply cleanly on fx-team anymore, it needs to be rebased. Can you make sure you update to the latest fx-team and rebase before submitting a new patch with the corrections from comment 23.
> 
> I think I made a mess. I rebased the patch this morning but I also commited
> the patch in the repo history. I surely don't have push access to the remote
> repository, so what do I do next?

Assuming you are using mq, you can `hg qpop -a` then `hg qimport -r 57e668dfdea3` and it will create a new patch in your queue with that commit (while removing it from the commit history in the repo).  Then you can pull and qpush it to fix any rejections / rebase.  There's a little more info about that here: https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Reference.
Attached patch patch candidate (obsolete) — Splinter Review
patch candidate

- removed extra empty line
- added comment to css property
- fixed extra indentation
Attachment #8616194 - Attachment is obsolete: true
Comment on attachment 8623272 [details] [diff] [review]
patch candidate

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

::: browser/themes/shared/devtools/images/animation-fast-track.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +

nit : Can this blank line be removed ?

@@ +5,5 @@
> +<svg
> +   xmlns="http://www.w3.org/2000/svg"
> +   width="9"
> +   height="12"
> +   docname="animation-fast-track.svg">

docname attribute isn't needed here.
Also, can you put all the attributes on the same line as the tagname ?

@@ +7,5 @@
> +   width="9"
> +   height="12"
> +   docname="animation-fast-track.svg">
> +  <g
> +     transform="matrix(1.0251088,0,0,0.85613344,-3.1546734,-888.94343)">

Same comment about the attributes.

@@ +9,5 @@
> +   docname="animation-fast-track.svg">
> +  <g
> +     transform="matrix(1.0251088,0,0,0.85613344,-3.1546734,-888.94343)">
> +    <path
> +       d="m 5.1284819,1038.3667 6.4950901,0 -2.7147491,4.6651 2.9438561,0 -8.1148915,9.3081 1.6126718,-6.8973 -2.2701022,0 z"

Same comment about the attributes

@@ +10,5 @@
> +  <g
> +     transform="matrix(1.0251088,0,0,0.85613344,-3.1546734,-888.94343)">
> +    <path
> +       d="m 5.1284819,1038.3667 6.4950901,0 -2.7147491,4.6651 2.9438561,0 -8.1148915,9.3081 1.6126718,-6.8973 -2.2701022,0 z"
> +       style="fill:#4cb0e1;fill-opacity:1;stroke:none"/>

fill-opacity and stroke properties are not needed, since they are set to their default value.
Assignee: nobody → edoardo.putti
Status: NEW → ASSIGNED
That last patch doesn't seem to be correct, it misses some changes. Let me try and clean this up a bit. I'll upload a new one shortly.
For info, here are 2 workflows with Mercurial that should help you avoid making a mess when rebasing/rewriting commits:

- With MQ:
  - (for info, Mercurial people advise against using MQ, because it's a very hacky way to play with Mercurial's internal data storage, but it's very convenient for working on Mozilla patches)
  - clone the repo:
    hg clone http://hg.mozilla.org/integration/fx-team
  - make your changes and create your first patch:
    hg qnew name-of-patch.diff
  - make more changes and integrate them in the same patch as you go:
    hg qref
  - change the commit message:
    hg qref -m "Bug 123456 - Something I changed"
  - after a while, you want to download the latest fx-team updates again, make sure you pop your patch
    hg qpop
    (or hg qpop -a if you have several patches applied)
    Never try to update the repo before poping your changes first.
  - update your repo:
    hg pull -u
  - re-apply your changes on top
    hg qpush
    (at this stage, if files you changed have changed in the repo too, you'll need to merge, MQ will save *.rej files that you'll need to merge by hand, run again hg qref when done).

- Without MQ, just using Mercurial and a few default extensions:
  - clone the repo:
    hg clone http://hg.mozilla.org/integration/fx-team
  - make your changes and create your first commit:
    hg commit -m "Bug 123456 - Something I changed"
  - make more changes and integrate them in the same commit as you go:
    hg commit -m "more changes"
    hg commit -m "more changes"
    hg histedit -r <revset of first commit you made>
    Then in the histedit editor, use "f" to fold all other commits into the first one.
    You can also use this step to change the commit message if needed.
  - after a while, you want to download the latest fx-team updates again, make sure you update to fx-team first
    hg up fx-team (I think using the fx-team tag name requires to have the firefoxtree extension, which I recommend http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/firefoxtree.html)
  - update your repo:
    hg pull fx-team -u
  - re-apply your changes on top
    hg up <revset of your commit>
    hg rebase -d fx-team
    (at this stage, you may need to merge, but unlike MQ, you'll have the possibility to use a real diff/merge tool here, like kdiff3 for example).
Attached patch bug1121896-compositor.patch (obsolete) — Splinter Review
So, I started from your previous patch, integrated your latest changes (from my last review) and also integrated the changes advised by Tim (thanks Tim!).
I ran a last couple of tests locally, everything seems ok.
Since the last TRY push was green, let's check this in.
Attachment #8623272 - Attachment is obsolete: true
Attachment #8623530 - Flags: review+
Keywords: checkin-needed
Just spotted something weird, there's an unrelated change in there, the removal of an image from a jar.mn file.
Keywords: checkin-needed
Attachment #8623530 - Attachment is obsolete: true
Attachment #8623534 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06d8b833c6a2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1176635
See Also: → 1205704
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.