Expose animation performance information in DevTools

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Animation Inspector
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: birtles, Assigned: ryo)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Bug 1196114 adds a new chrome-only API that provides the following information:

1. *Which* animation properties are being run on the compositor (until now there has just been a single flag say if one or more of the properties is running on the compositor)

2. For animations that normally *could* be run on the compositor but aren't, the reason why.

An example of 2 is if the is an element where we're animating the 'left' property at the same time as the 'transform' property, we'll deliberately *not* run the transform animation on the compositor because it will look odd because the different things affecting the element's position won't be synchronized.

(Some background I forgot to add: currently *only* 'transform' and 'opacity' can be animated on the compositor. In future, we'll add 'scale', 'translate', 'rotate' etc.)

Another example of 2, is if we animate 'transform' but the element has an SVG transform attribute or a backface-visibility property then we don't do the animation on the compositor due to limitations in Gecko.

In any case, the idea is that by exposing that information to developers, they can analyze and optimize their content. e.g. Perhaps when we hit one of those type-2 scenarios, the lightning bolt could be outlined and have a tooltip explaining why the property wasn't run on the compositor?

CC'ing Helen since she might have some ideas about how this could look.
For information, we need to be careful about backwards compatibility when it comes to devtools:

for example, a user might be using devtools in Firefox 47, but connect them to an older Firefox (indeed, you can debug Firefox for Android remotely, or even another Firefox desktop process remotely).
This means that you may be using the user interface of devtools from version N, but connect this UI to the server of devtools from version N-1.

In this bug for instance, we might be tempted to remove all occurrences of isRunningOnCompositor in devtools\server\actors\animation.js and replace them with the new information coming from AnimationPropertyState, but if we did that, then a recent UI that expects this new information, but connected to an older server, won't be getting it.

So, for the devtools side (unless this bug is only for the platform, and not for devtools), I suggest doing the following:
- keep the isRunningOnCompositor in devtools\server\actors\animation.js, but instead of initializing it from Animation.isRunningOnCompositor, set it to true or false depending on what AnimationPropertyState tells us,
- add new data to the object returned by AnimationPlayerActor.getState according to AnimationPropertyState,
- on the UI side (in devtools\client\animationinspector\components\*), keep on using isRunningOnCompositor, but display more information if the server sends it (old servers will just be sending the isRunningOnCompositor boolean, but new servers that have access to AnimationPropertyState will be sending more information).
In terms of UX, here's what I can think of:

Right now we have a tooltip that's displayed when you hover over an animation in devtools. It is used now
to dispay the duration, iterationcount, delay, name, type, etc... We could also append this information in it. 
Or we could make it more obvious, with a dedicated tooltip shown when hovering over the lightning bolt icon?
Priority: -- → P2
Whiteboard: [btpp-fix-later]
(Reporter)

Updated

a year ago
Assignee: nobody → motoryo1
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Created attachment 8730077 [details]
animationinspector.png

Brian and I talked about this bug. And I made a screenshot of animation inspector.
Half colored thunderbolt icon on animation block means more than one properties are associated with this animation and some properties are running on compositor but some properties are not running on compositor.
And thunderbolt icon displayed on the side of each property name means which they are running on compositor or not.
 - Green thunderbolt icon means it is running on compositor.
 - Gray icon means it is not running on compositor and warnings are reported about why it is not running on compositor.
 - No icon are displayed, it is not running on compositor and no warnings are reported.
When we position the mouse pointer over a gray thunderbolt icon, display the warning sentence.

My plan about UX is like this. Please give opinion to me.
Flags: needinfo?(pbrosset)
(Reporter)

Updated

a year ago
Depends on: 1223204
(Reporter)

Comment 4

a year ago
I think this looks good. One minor unrelated note, I think we shouldn't show "delay" and "endDelay" in the tooltip when they are zero.
(In reply to Ryo Motozawa [:ryo] from comment #3)
> Created attachment 8730077 [details]
> animationinspector.png
> 
> Brian and I talked about this bug. And I made a screenshot of animation
> inspector.
> Half colored thunderbolt icon on animation block means more than one
> properties are associated with this animation and some properties are
> running on compositor but some properties are not running on compositor.
> And thunderbolt icon displayed on the side of each property name means which
> they are running on compositor or not.
>  - Green thunderbolt icon means it is running on compositor.
>  - Gray icon means it is not running on compositor and warnings are reported
> about why it is not running on compositor.
>  - No icon are displayed, it is not running on compositor and no warnings
> are reported.
> When we position the mouse pointer over a gray thunderbolt icon, display the
> warning sentence.
> 
> My plan about UX is like this. Please give opinion to me.
Sorry about the delay, this week has been a bit difficult for me, some of the devtools team was having a dedicated work week.

I like the screenshot, it looks pretty good!
I generally agree with the ideas, I just have a couple of comments:
- On the animation block, instead of a half-colored bolt, I think maybe a different color bolt would be better.
  - no icon: not on running on compositor
  - greyed-out icon: some properties only are running on the compositor
  - normal icon: all properties run on the compositor
Maybe that means we need a new color for the normal icon because it is simply white today, so grey vs. white will make it hard to see the difference.
- If we do that, then we can reuse the same thing for each animated property (as you did in your screenshot): not on compositor -> greyed-out bolt, on compositor -> normal icon

Having said this, I'd still like Helen's point of view on this design proposal.
I guess the counter argument would be that it's a lot of "technical" information to add to the UI. Note that at some stage we also want to display animated property values in the sidebar, so we should reserve space for those too.
Maybe having the information only displayed in the tooltip that appears when you hover over the bolt in the animation block would be enough? And we can skip adding icons for each animated properties? (we could have a dedicated tooltip for that bolt icon by the way, that contains all the warnings).
Flags: needinfo?(pbrosset) → needinfo?(hholmes)
(Reporter)

Comment 6

a year ago
> I like the screenshot, it looks pretty good!
> I generally agree with the ideas, I just have a couple of comments:
> - On the animation block, instead of a half-colored bolt, I think maybe a
> different color bolt would be better.
>   - no icon: not on running on compositor
>   - greyed-out icon: some properties only are running on the compositor

What about an outlined thunderbolt (i.e. white stroke only)? Perhaps even a dotted stroke? That might preserve the binary color scheme we have going there (green and white only; or, for other animation type yellow/white etc.)

>   - normal icon: all properties run on the compositor
> Maybe that means we need a new color for the normal icon because it is
> simply white today, so grey vs. white will make it hard to see the
> difference.
> - If we do that, then we can reuse the same thing for each animated property
> (as you did in your screenshot): not on compositor -> greyed-out bolt, on
> compositor -> normal icon

Actually, I think the idea was you'd have three states per property:

* no icon = not running on the compositor but nor do we expect it to (e.g. a color animation--the screenshot is misleading here)
* grey / outlined icon = not running on the compositor but *could* be if the author tweaked their content (e.g. layer is too large). The author can hover over the icon to find out what the issue is.
* green / full icon = running on the compositor

So in the common case, you'll just see a full icon against opacity/transform properties and nothing next to other properties.

I kind of like having the thunderbolt next to opacity/transform because it allows authors to very naturally learn, "Oh, opacity and transform are fast, I should use them more often".

> Having said this, I'd still like Helen's point of view on this design
> proposal.
> I guess the counter argument would be that it's a lot of "technical"
> information to add to the UI.

Yeah, I was wondering that too. However, if we don't show any icon for the case when the animation is not running on the compositor, I think it makes the screen less busy / overwhelming?

> Maybe having the information only displayed in the tooltip that appears when
> you hover over the bolt in the animation block would be enough? And we can
> skip adding icons for each animated properties? (we could have a dedicated
> tooltip for that bolt icon by the way, that contains all the warnings).

That might work. I guess I'd like to see it, though. It sounds like it could a really big tooltip!
(In reply to Patrick Brosset [:pbro] from comment #5)
> (In reply to Ryo Motozawa [:ryo] from comment #3)
> > Created attachment 8730077 [details]
> > animationinspector.png
> I guess the counter argument would be that it's a lot of "technical"
> information to add to the UI. Note that at some stage we also want to
> display animated property values in the sidebar, so we should reserve space
> for those too.
I think that this is a lot of technical information to add to the UI. When there are a bunch of lightning bolts all over the animation inspector, they cease to have meaning.

> Maybe having the information only displayed in the tooltip that appears when
> you hover over the bolt in the animation block would be enough? And we can
> skip adding icons for each animated properties? (we could have a dedicated
> tooltip for that bolt icon by the way, that contains all the warnings).
I think using color in the lightning bolts to express either a) all properties are on the compositor or b) some properties are on the compositor with a tooltip explaining the warnings sounds like a good compromise for now. As the animation tool adds new functionality it might make more sense elsewhere, but this seems like it would be less work and UI for now.
Flags: needinfo?(hholmes)
(Assignee)

Comment 8

a year ago
Created attachment 8733708 [details]
screeenshot_tooltip.png

Thank you all for information.

In this screen shot, Opacity has no waring but transform has a warning.
I think thunderbolt icons on sidebar is need to show which animation has a warning.
(Reporter)

Comment 9

a year ago
(In reply to Ryo Motozawa [:ryo] from comment #8)
> Created attachment 8733708 [details]
> screeenshot_tooltip.png
> 
> Thank you all for information.
> 
> In this screen shot, Opacity has no waring but transform has a warning.
> I think thunderbolt icons on sidebar is need to show which animation has a
> warning.

I spoke with Ryo about Helen's feedback this morning and we have two questions:

1) We think it is too much information to put *all* of it in one tooltip so Ryo's screenshot shows how we could split up the tooltips: one summary when you hover over the lightning bolt, and more specific details for each property when you hover over it.

However, if we drop the lightning bolt next to the property names, we need a way to the user that there is some important information if they hover of the property, particularly for the case when the animation is not running on the compositor but *could* me.

We wondered about possibly making having a dotted underline under those properties? So, in the screenshot, the string 'transform' would have a dotted underline. What do you think?

2) For the different colored lightning bolts, I'm not sure what colors would make sense--particularly since animations can be green / orange / blue. Black / white lightning bolts seem cryptic. Perhaps we fully white lightning bolt (i.e. white fill) vs a white outlined lightning bolt (i.e. white stroke)?
(Assignee)

Comment 10

a year ago
Created attachment 8733741 [details]
thunderbolt_icons.png

I made thunderbolt icons sample.
(Reporter)

Comment 11

a year ago
Helen, what do you think of the two screenshots and comments 8 - 10?
Flags: needinfo?(hholmes)
I like the idea of a dotted underline to separate the idea of on the compositor vs. not on the compositor. (Having the lightning bolt on non-compositor properties makes it seem like they are fast + on the compositor when they are not—confusing!).

Looking at Ryo's screenshot, I would propose swapping out the gray thunderbolts next to non-on-compositor properties for dotted underlines, keeping the gray thunderbolt on the top-level (so, agreeing with Brian's suggestion).
Flags: needinfo?(hholmes)
(Reporter)

Comment 13

a year ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #12)
> I like the idea of a dotted underline to separate the idea of on the
> compositor vs. not on the compositor. (Having the lightning bolt on
> non-compositor properties makes it seem like they are fast + on the
> compositor when they are not—confusing!).

Thanks Helen! Just to clarify, we have three categories here:

A) Running on the compositor
B) Not running on the compositor but there's a warning saying why (i.e. this animation *should* be able to be run on the compositor but for some reason it's not--so the author should tweak their content so it can)
C) Not running on the compositor (and not expected to either)

So is there proposal here:

1.
A) Lightning bolt
B) Dotted underline
C) Nothing

2.
A) Dotted underline
B) Nothing
C) Nothing

3.
A) Dotted underline
B) Dotted underline
C) Nothing

4.
A) Solid underline
B) Dotted underling
C) Nothing

From my understanding of the comment, it's (2) from above, but that seems odd to me. For the (B) case, how will the author know to hover over the property to discover the important information? I thought the purpose of the dotted underline was a visual clue, "Hey, there's more information here if you hover"

Perhaps (A) is better? Or something else altogether?
Flags: needinfo?(hholmes)
(In reply to Brian Birtles (:birtles) from comment #13)
> 1.
> A) Lightning bolt
> B) Dotted underline
> C) Nothing

Lightning bolt for "on the compositor" --> full color if everything is on the compositor, gray if only some properties are.

Dotted underlines when expanded for properties that could be on the compositor, but are not.

Nothing for things that are not on the compositor and cannot be on the compositor.
Flags: needinfo?(hholmes)
(Reporter)

Comment 15

a year ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #14)
> (In reply to Brian Birtles (:birtles) from comment #13)
> > 1.
> > A) Lightning bolt
> > B) Dotted underline
> > C) Nothing
> 
> Lightning bolt for "on the compositor" --> full color if everything is on
> the compositor, gray if only some properties are.

Does this refer to the expanded property view? I guess it must but then I'm not sure how they could be gray. (And if it doesn't, then I'm not sure how we're to distinguish properties running on the compositor from those which aren't.)  We'll just go ahead and implement (1) and sort it out in the review process.
(Assignee)

Comment 16

a year ago
Created attachment 8735751 [details]
MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

[mq]: Expose_animation_performance_information_in_DevTools

Review commit: https://reviewboard.mozilla.org/r/42937/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42937/
Attachment #8735751 - Flags: review?(pbrosset)
Attachment #8735752 - Flags: review?(pbrosset)
(Assignee)

Comment 17

a year ago
Created attachment 8735752 [details]
MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

[mq]: Expose_animation_performance_information_in_DevTools_test

Review commit: https://reviewboard.mozilla.org/r/42939/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42939/
Blocks: 1260665
Comment on attachment 8735751 [details]
MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

https://reviewboard.mozilla.org/r/42937/#review39717

This works fine, and I like the outline and lightning bolts. I think we should go with this proposal for the UI.
I do have a few comments about the code though. See below. I believe we aren't far though, there are no big changes to be made.

::: devtools/client/animationinspector/components/animation-details.js:142
(Diff revision 1)
> -
> +      let warning = "";
> +      let classname = "";
> +      if (animation.state.propertyState) {
> +        let runningOnCompositor;
> +        for (let prs of animation.state.propertyState) {
> +          if (prs.property == propertyName) {
> +            runningOnCompositor = prs.runningOnCompositor;
> +            if (typeof prs.warning != "undefined") {
> +              warning = prs.warning;
> +            }
> +          }
> +        }
> +        if (runningOnCompositor && warning == "") {
> +          classname = "oncompositor";
> +        } else if (!runningOnCompositor && warning != "") {
> +          classname = "warning";
> +        }
> +      }

Please move all of this logic into a separate method. Maybe something called 

`getPerfDataForProperty: function(aniamtion, propertyName)`

It would return something like:

`return {className, warning};`

::: devtools/client/animationinspector/components/animation-details.js:145
(Diff revision 1)
>          attributes: {"class": "property"}
>        });
> -
> +      let warning = "";
> +      let classname = "";
> +      if (animation.state.propertyState) {
> +        let runningOnCompositor;

If this is meant to be a boolean, then it should be named `isRunningOnCompositor`, it makes the intent clearer.

::: devtools/client/animationinspector/components/animation-details.js:146
(Diff revision 1)
>        });
> -
> +      let warning = "";
> +      let classname = "";
> +      if (animation.state.propertyState) {
> +        let runningOnCompositor;
> +        for (let prs of animation.state.propertyState) {

The variable name `prs` is a bit cryptic, and further below, you're using a different one `pst`. I think you should just use `propertyState` (or potentially `propState` if that is too long) and use it consistently.

::: devtools/client/animationinspector/components/animation-details.js:151
(Diff revision 1)
> +        for (let prs of animation.state.propertyState) {
> +          if (prs.property == propertyName) {
> +            runningOnCompositor = prs.runningOnCompositor;
> +            if (typeof prs.warning != "undefined") {
> +              warning = prs.warning;
> +            }

You should `break;` after this if, since we only expect one item in the propertyState array for this propertyName, no need to continue looping further.

::: devtools/client/animationinspector/components/animation-details.js:166
(Diff revision 1)
>          // text-overflow doesn't work in flex items, so we need a second level
>          // of container to actually have an ellipsis on the name.
>          // See bug 972664.
>          parent: createNode({
>            parent: line,
> -          attributes: {"class": "name"},
> +          attributes: {"title": warning,

It would make more sense to have the title attribute on the same node where the warning/compositor class is. So remove the attribute from here, and move it to the attributes of the child node isntead.

::: devtools/client/animationinspector/components/animation-time-block.js:179
(Diff revision 1)
>      }
>  
>      // Adding a note that the animation is running on the compositor thread if
>      // needed.
> -    if (state.isRunningOnCompositor) {
> +    if (state.propertyState) {
> +      if (state.propertyState.every(pst => pst.runningOnCompositor)) {

Renaming `pst` into `propState` here will cause an ESLint warning since the line will be longer than 80 chars.
You could rewrite this if/else condition to this:

```
text +=
  state.propertyState.every(propState => propState.runningOnCompositor)
  ? L10N.getStr("player.isRunningOnCompositorTooltip")
  : L10N.getStr("player.notRunningOnCompositorTooltip");
```

::: devtools/client/animationinspector/components/animation-time-block.js:179
(Diff revision 1)
> +      if (state.propertyState.every(pst => pst.runningOnCompositor)) {
> +        text += L10N.getStr("player.isRunningOnCompositorTooltip");
> +      } else {
> +        text += L10N.getStr("player.notRunningOnCompositorTooltip");
> +      }

With this code, animations that aren't running on the compositor at all (no properties at all) will still have the message "Not all of animation properties are running on the compositor".

This implies that maybe some properties are.
I think we should have 3 conditions:
- either all are running 
- or some are running
- or none are running

If none, then we shouldn't have a message at all.

::: devtools/client/animationinspector/components/animation-timeline.js:287
(Diff revision 1)
>  
>      this.drawHeaderAndBackground();
>  
>      for (let animation of this.animations) {
>        animation.on("changed", this.onAnimationStateChanged);
> -
> +      let compositorallornot = "";

Variable names should be in camelcase. Also I think the name isn't the best. This is going to be used a className on the node, so the name should probably reflect this. Maybe `compositorStatusClass`.

::: devtools/client/animationinspector/components/animation-timeline.js:287
(Diff revision 1)
> -
> +      let compositorallornot = "";
> +      if (animation.state.propertyState) {
> +        let runningOnCompositor = true;
> +        compositorallornot = " compositor-all";
> +        for (let pst of animation.state.propertyState) {
> +          runningOnCompositor = pst.runningOnCompositor;
> +          if (!runningOnCompositor) {
> +            compositorallornot = " compositor-notall";
> +          }
> +        }
> +      }

Similar to another comment I made earlier, let's move all of this to another method on the class.

Maybe something like this:

```
getCompositorStatusClassName: function({state}) {
  let className = state.isRunningOnCompositor
                  ? " fast-track"
                  : "";
 
  if (state.isRunningOnCompositor && state.propertyState) {
    className +=
      state.propertyState.some(propState => !propState.runningOnCompositor)
      ? " some-properties"
      : " all-properties";
  }
  
  return className;
}
```

This way, everything that has to do with compositor info is all grouped into one method, and you can simplify the code inside `createNode` below a bit.

Note that I have changed the `if` condition by adding a check on `state.isRunningOnCompositor`. This is so that if the animation isn't running on the compositor at all, I guess the "some-properties" class isn't needed, right?

::: devtools/client/animationinspector/components/animation-timeline.js:306
(Diff revision 1)
>          parent: this.animationsEl,
>          nodeType: "li",
>          attributes: {
>            "class": "animation " +
>                     animation.state.type +
> -                   (animation.state.isRunningOnCompositor ? " fast-track" : "")
> +                   (animation.state.isRunningOnCompositor ?

I don't remember why we generate the "fast-track" class here in the AnimationsTimeline component. This really belongs into AnimationTimeBlock instead.
I'll file another to move all of the compositor related code into this other component. In the meantime, it's fine to have it here, since it was here before.

::: devtools/client/locales/en-US/animationinspector.properties:97
(Diff revision 1)
>  # LOCALIZATION NOTE (player.runningOnCompositorTooltip):
>  # This string is displayed as a tooltip for the icon that indicates that the
>  # animation is running on the compositor thread.
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (player.isRunningOnCompositorTooltip):

This string name is too similar to the previous one `runningOnCompositorTooltip`.
Maybe `allPropertiesOnCompositorTooltip` ?
And for the next one: `somePropertiesOnCompositorTooltip` ?

::: devtools/client/themes/animationinspector.css:403
(Diff revision 1)
>    height: 100%;
>    width: var(--fast-track-icon-width);
>    z-index: 1;
> +}
>  
> -  background-image: url("images/animation-fast-track.svg");
> +.compositor-all .name::after {

As suggested in an earlier comment, I think this classname should be `.all-properties` instead (and `.some-properties` for the next one).

Also, for consistency, please scope this into the `.animation-timeline` class.

::: devtools/server/actors/animation.js:247
(Diff revision 1)
>        iterationStart: this.getIterationStart(),
>        // animation is hitting the fast path or not. Returns false whenever the
>        // animation is paused as it is taken off the compositor then.
> -      isRunningOnCompositor: this.player.isRunningOnCompositor,
> +      isRunningOnCompositor:
> +        this.player.effect.getProperties().some(pst => pst.runningOnCompositor),
> +      propertyState: this.player.effect.getProperties(),

Note that the animation tool sends this state object to the client-side (the timeline) everytime the UI needs to be refreshed, either when a new animation is added, or the page is reloaded, or a different node is selected in the inspector, etc.
So, we try and keep it small, because it goes through the devtools protocol.
Transferring small amounts of data is important when debugging a remote device via USB for example.

I've inspected the protocol traffic using the RDP inspector addon (https://github.com/firebug/rdp-inspector) and saw that we end up sending more than we need. In particular, each property has a `values` sub-property that contains the keyframes data. We don't need this here (other parts of devtools need it, but they get it separately).

So, please create a new method on the class named `getPropertiesCompositorStatus` and in this method, call `this.player.effect.getProperties` and filter its output so that we only send an array of `{property, runningOnCompositor, warning}`.
Attachment #8735751 - Flags: review?(pbrosset)
Comment on attachment 8735752 [details]
MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

https://reviewboard.mozilla.org/r/42939/#review39735

Looks fine, apart from one test which isn't doing what's expected.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_displayed.js:77
(Diff revision 1)
> +function hasExpectedWarnings(containerEl) {
> +  let warnings = [...containerEl.querySelectorAll(".property .name")]
> +                 .filter(w => w == "transition");
> +  for (let warning of warnings) {
> +    if (warning.getAttribute("title") !==
> +         L10N.getStr("AnimationWarningTransformWithGeometricProperties")) {

L10N isn't defined in this file, so running this test will fail with a JS error.
In fact, the test works for me locally, but that's because we never go in the `if`, the `warnings` array is empty, so the test isn't testing what you think it should.

Looks like this string is part of this bundle: layout_errors.properties

You can do something like this to import it:

const { LocalizationHelper } = require("devtools/client/shared/l10n");
const STRINGS_URI = "chrome://global/locale/layout_errors.properties";
const L10N = new LocalizationHelper(STRINGS_URI);
Attachment #8735752 - Flags: review?(pbrosset)
(Assignee)

Comment 20

a year ago
Comment on attachment 8735751 [details]
MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42937/diff/1-2/
Attachment #8735751 - Flags: review?(pbrosset)
Attachment #8735752 - Flags: review?(pbrosset)
(Assignee)

Comment 21

a year ago
Comment on attachment 8735752 [details]
MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42939/diff/1-2/
Comment on attachment 8735751 [details]
MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

https://reviewboard.mozilla.org/r/42937/#review40325

r=me with potentially my first comment addressed, depending on what what propertyState contains.

::: devtools/client/animationinspector/components/animation-details.js:70
(Diff revisions 1 - 2)
> +      for (let propState of animation.state.propertyState) {
> +        if (propState.property == propertyName) {
> +          isRunningOnCompositor = propState.runningOnCompositor;
> +          if (typeof propState.warning != "undefined") {
> +            warning = propState.warning;
> +            break;

Shouldn't this `break` be one block up, in the parent `if` ?
I'm not familiar with what can be in the propertyState array, but I was assuming that there would be only one item per property name. If that is indeed the case, then you should break as soon as you found that propertyName.

::: devtools/server/actors/animation.js:254
(Diff revisions 1 - 2)
> +    for (let propState of this.player.effect.getProperties()) {
> +      propertyStates.push({
> +        property: propState.property,
> +        runningOnCompositor: propState.runningOnCompositor,
> +        warning: propState.warning
> +      });
> +    }

nit: with some es6 goodness, you can also write this:

```
for (let {property, runningOnCompositor, warning} of this.player.effect.getProperties()) {
  propertyStates.push({property, runningOnCompositor, warning});
}
```

But that might make it hard to fit in under 80 chars. So up to you.

Also, you could make the whole method even simpler by using Array.map:

```
getPropertiesCompositorStatus: function() {
  let properties = this.player.effect.getProperties();
  return properties.map(prop => {
    return {
      property: prop.property,
      runningOnCompositor: prop.runningOnCompositor,
      warning: prop.warning
    };
  });
}
```

But again, these are minor comments, up to you.
Attachment #8735751 - Flags: review?(pbrosset) → review+
Attachment #8735752 - Flags: review?(pbrosset) → review+
Comment on attachment 8735752 [details]
MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

https://reviewboard.mozilla.org/r/42939/#review40355

r=me with my comment addressed.
Otherwise, if a test change is needed to make sure this works, and these changes are simple, no need to re-request a review.

::: devtools/client/animationinspector/test/browser_animation_animated_properties_displayed.js:79
(Diff revision 2)
>  }
> +
> +function hasExpectedWarnings(containerEl) {
> +  let warnings = [...containerEl.querySelectorAll(".property .name")]
> +                 .filter(w => w == "transition");
> +  for (let warning of warnings) {

Can you please confirm that `warnings` isn't always an empty array here? We want to make sure we actually test this correctly, and last time I ran this test, it was not getting into the loop because `warnings` was empty.
(Reporter)

Comment 24

a year ago
https://reviewboard.mozilla.org/r/42937/#review43283

::: devtools/client/locales/en-US/animationinspector.properties:94
(Diff revision 2)
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (player.allPropertiesOnCompositorTooltip):
> +# This string is displayed as a tooltip for the icon that indicates that
> +# all of animation is running on the compositor thread.
> +player.allPropertiesOnCompositorTooltip=All of animation properties are running on compositor thread

As discussed in bug 1255683, we should make this:

"All animation properties are optimized"

::: devtools/client/locales/en-US/animationinspector.properties:99
(Diff revision 2)
> +player.allPropertiesOnCompositorTooltip=All of animation properties are running on compositor thread
> +
> +# LOCALIZATION NOTE (player.somePropertiesOnCompositorTooltip):
> +# This string is displayed as a tooltip for the icon that indicates that
> +# all of animation is not running on the compositor thread.
> +player.somePropertiesOnCompositorTooltip=Not all of animation properties are running on compositor thread

And we should make this, "Some animation properties are optimized"
(Reporter)

Comment 25

a year ago
Motozawa-san, I hope you are well. Will you have time to look at this next week? It seems like it is *nearly* ready to land. It would be good to land this before the next uplift (2016-04-25) so this is part of Firefox 48.

If you don't have time, please let me know--someone else can probably fix the remaining review feedback.
Flags: needinfo?(motoryo1)
(Assignee)

Comment 26

a year ago
I apologize for this late reply. I'll fix this bug in this week.
Flags: needinfo?(motoryo1)
(Assignee)

Comment 27

a year ago
Comment on attachment 8735751 [details]
MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42937/diff/2-3/
Attachment #8735751 - Attachment description: MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r?pbro → MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro
Attachment #8735752 - Attachment description: MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r?pbro → MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro
(Assignee)

Comment 28

a year ago
Comment on attachment 8735752 [details]
MozReview Request: Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42939/diff/2-3/
Ryo, can you please take time to fix failures on try?
https://treeherder.mozilla.org/logviewer.html#?job_id=19853973&repo=try
Flags: needinfo?(motoryo1)
ni to me since ryo tole me on IRC that he could not reproduce the failures locally and has no enough time to look the failures in detail.
Flags: needinfo?(motoryo1) → needinfo?(hiikezoe)
https://reviewboard.mozilla.org/r/42939/#review45887

::: devtools/client/animationinspector/test/browser_animation_animated_properties_displayed.js:77
(Diff revisions 2 - 3)
> -  let warnings = [...containerEl.querySelectorAll(".property .name")]
> +  let warnings = [...containerEl.querySelectorAll(".warning")];
> -                 .filter(w => w == "transition");
>    for (let warning of warnings) {
> -    if (warning.getAttribute("title") !==
> +    if (warning.getAttribute("title") ==
>           L10N.getStr("AnimationWarningTransformWithGeometricProperties")) {
>        return false;
>      }
>    }
>    return true;

Considering Patrick's review comment in comment #23, Ryo mightt want to do here like this.

  for (let warning of warnings) {                                               
    if (warning.getAttribute("title") ==                                        
         L10N.getStr("AnimationWarningTransformWithGeometricProperties")) {     
      return true;                                                              
    }                                                                           
  }                                                                             
  return false;
  
  I just pushed a try with this change.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=401ee237ea87
The try result gets better but still has failures on Linux debug builds.
The failure reason is a short duration of the animation.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=604758ca0cc3

Patrick, can you please review an additional patch here?  I can not update Ryo's patches and upload the additional patch without clearing r+ against Ryo's patches on review-board.
Flags: needinfo?(hiikezoe)
Created attachment 8746320 [details] [diff] [review]
Bug 1254408 - Part3 Use longer duration to avoid intermittent failures
Attachment #8746320 - Flags: review?(pbrosset)
Comment on attachment 8746320 [details] [diff] [review]
Bug 1254408 - Part3 Use longer duration to avoid intermittent failures

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

Seems fine to me. I see no tests in the tree that wait for this animation to end, so setting it to 100s should be fine.
Attachment #8746320 - Flags: review?(pbrosset) → review+
Created attachment 8747967 [details] [diff] [review]
Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro

Fix two lint errors.
Attachment #8735751 - Attachment is obsolete: true
Attachment #8747967 - Flags: review+
Created attachment 8747968 [details] [diff] [review]
Bug 1254408 - Part2 Modifing tests for animation property warnings r=pbro

Fix a failure described in comment 31.
Attachment #8735752 - Attachment is obsolete: true
Attachment #8747968 - Flags: review+
Attachment #8747967 - Attachment description: MozReview Request: Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro → Bug 1254408 - Part1 Expose animation performance information in DevTools r=pbro
Attachment #8746320 - Attachment description: Part 3: Use longer duration to avoid intermittent failures → Bug 1254408 - Part3 Use longer duration to avoid intermittent failures
Keywords: checkin-needed
Attachment #8747968 - Attachment is patch: true

Comment 38

a year ago
https://hg.mozilla.org/integration/fx-team/rev/4a9ae4f8921d
https://hg.mozilla.org/integration/fx-team/rev/9b5bfd9c7565
https://hg.mozilla.org/integration/fx-team/rev/791d030d9034
Keywords: checkin-needed
This will need some doc updates.
There are now 3 things: a lightning bolt icon on the animation itself, lightning bolts next to each properties (when applicable), and underlined properties with warnings as tooltips when there is a perf warning to be displayed.
Keywords: dev-doc-needed

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a9ae4f8921d
https://hg.mozilla.org/mozilla-central/rev/9b5bfd9c7565
https://hg.mozilla.org/mozilla-central/rev/791d030d9034
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
I have documented these new features in the following section:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Work_with_animations#Further_information_about_animation_compositing

I've also made sure a note has been added to the relevant Firefox developer release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools

A quick review would be great - thanks!
Keywords: dev-doc-needed → dev-doc-complete
Great!  All of the examples are really nice.

One nit I've noticed is that the color in below sentence was not correct.
  "The expanded animation information now includes a green lightning"

The screenshot corresponding to the sentence has an orange icon.  I've already fixed the color.

Note that, though you may already know, the color of the icon represents types of animations.

 orange: CSS animations
 blue: CSS transitons
 green: Web animations

Anyway, thanks for the great document!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> Great!  All of the examples are really nice.
> 
> One nit I've noticed is that the color in below sentence was not correct.
>   "The expanded animation information now includes a green lightning"
> 
> The screenshot corresponding to the sentence has an orange icon.  I've
> already fixed the color.
> 
> Note that, though you may already know, the color of the icon represents
> types of animations.
> 
>  orange: CSS animations
>  blue: CSS transitons
>  green: Web animations
> 
> Anyway, thanks for the great document!

Heh, thanks for catching this - this is my color blindness in full effect ;-)

I have changed the description to just "a lightning bolt", as it will be a different color depending on the type of animation.
You need to log in before you can comment on or make changes to this bug.