1210795 - Display animations' timing-functions in the animation-inspector

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Bug 1197100 makes animations displayed in the animation-inspector panel selectable. Once they are selected, keyframes information are displayed.
We should also display the animation's timing function in there somewhere.

We could even leverage the cubic-bezier editor tooltip to let users edit the function directly from there.

Comment 1

[Patrick Brosset <:pbro>]

Bug triage (filter on CLIMBING SHOES).

Comment 2

[Brian Birtles (:birtles)]

Patrick, Daisuke is interested in working on a representation for this based on the proposal we had here:

  http://codepen.io/birtles/pen/OXVebR

He'd like to make up a prototype. Is that ok with you, or do you have someone else interested in working on it?

Also, I seem to remember there being some concerns about the performance of the animation panel. Do you know what in particular was heavyweight?

If we go ahead and make up a prototype, I'm wondering if drawing the wave shape with canvas might be more lightweight than SVG (especially when you use shorthands and have a lot of potentially overlapping shapes). So I thought I'd check if you already had plans for drawing larger parts of the panel with canvas.

Comment 3

[Brian Birtles (:birtles)]

(In reply to Patrick Brosset <:pbro> from comment #0)
> We could even leverage the cubic-bezier editor tooltip to let users edit the
> function directly from there.

Just a thought, but I wonder if we could layer the cubic-bezier editor directly onto the representation of the timing function.

In the codepen we put forward at London[1], once you click the combined graph it expands into two areas: timing graph (top) and property graphs (bottom). The idea being that the combined graph is read-only but once you expand it, both parts are editable. So, in the example, in the expanded view, when you hover over the grey graph at the top, we could pop-up little bezier handles directly on the graph itself like in this sketch:

  http://slides.com/birtles/devtools-london/#/2/11

There would be some tricky cases: If less than a full iteration is played, you might need to show the rest of the iteration in some sort of partially transparent fashion so that you can put handles on it. Likewise, if you have direction:reverse, you'd have to be clever enough to reverse the handles. And then we'd have to work out what to do for step timing functions. Still, if we can manage that, I think it would be more intuitive that another pop-up.

[1] http://codepen.io/birtles/pen/OXVebR

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #2)
> Patrick, Daisuke is interested in working on a representation for this based
> on the proposal we had here:
> 
>   http://codepen.io/birtles/pen/OXVebR
> 
> He'd like to make up a prototype. Is that ok with you, or do you have
> someone else interested in working on it?
No I don't, feel free to take this bug!
A prototype based on your proposal would be great!

> Also, I seem to remember there being some concerns about the performance of
> the animation panel. Do you know what in particular was heavyweight?
I don't remember anything specific. The animation panel is currently implemented using HTML/CSS only, and I don't think we've had cases where that caused performance issues.
Each animation uses a certain number of DOM nodes, and then CSS linear-gradients are used to represent iterations. So, in theory, if you had many many animations, then you could potentially have performance issues.
In most cases, the number of animations displayed is really low, and once we get grouping (bug 1210363), the number will be even lower.

> If we go ahead and make up a prototype, I'm wondering if drawing the wave
> shape with canvas might be more lightweight than SVG (especially when you
> use shorthands and have a lot of potentially overlapping shapes). So I
> thought I'd check if you already had plans for drawing larger parts of the
> panel with canvas.
This was indeed considered at various times, but never done. As I said, we never really hit a serious performance problem, nor did we need to display crazy graphics so far, so HTML and CSS did the trick.
The more complex graphs you intend to prototype might indeed require canvas, but migrating the panel to canvas is a complex project in its own, that will need to be done even before you can work on this bug at all. Unless you want to have one <canvas> per animation, in which case it would be a simpler migration path.

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: v0.webm

I attach the prototype movie.

Comment 6

[Brian Birtles (:birtles)]

Hi Patrick, I wonder what you think of attachment 8770434 [details]. There are a few ideas there for how this view might look.

A few points of interest:

* It starts off with a summary view that is supposed to show the overall pattern of the animation and give you a sense for what properties are involved. We'd use the same color for the same property (e.g. transform is always orange etc.) so you can get an idea at a glance. Colors / opacity show the color/opacity in the graph. I'm not sure how we'll distinguish between different color-type properties (e.g. if you animate both 'background-color' and 'color' at the same time). I'm also not sure how we'd distinguish between different types of animations (e.g. transitions vs CSS animations) with this view -- perhaps we'd could use the background color.

Once you click the summary view it expands to show the individual properties in the same way as the current panel as well as the timing profile. Having a summary view probably matters most when shorthands are being used since they expand to a lot of subproperties.

* It tries to represent properties on a natural scale so that an opacity of 1 is at the top of the graph and an opacity of 0 is at the bottom. For colors, the height is related to the hue (this might need some work in case you animate between two colors with different luminance/saturation but with a hue of zero). For transforms the idea is we'd expand translateX / translateY / scale etc. into separate graphs that are linked together (i.e. tweaking one simultaneously updates the others). Where splitting a transform out is not possible we'd just fall back to using cumulative distance from the start point to represent the height.

* It includes handles to show how it might be possible to adjust timing parameters from this view. It's a bit buggy, and there would need to be more space between the graphs so you can drag the handles further, but if this approach worked, we'd probably apply similar treatment to the individual property graphs too for editing per-keyframe easing / keyframe offsets etc. As for switching between different *types* of easing (e.g. between cubic-bezier and steps) we might need an extra panel I guess.

Some other things we're not sure about:

* I'm not sure if using the same natural scale for the summary view makes sense. It means that the height of the summary view doesn't necessarily have much corresponding to progress. As a result, you can't clearly see animation-direction, or even iterations. Also, when you tweak the timing graph, the result is often surprising. I wonder if using cumulative distance for the top graph would be better.

* I'm not sure how we should handle shorthands. The getProperties() API returns the expanded longhands with computed values but when it comes to editing, in some cases it would be easier to edit the property as a shorthand and with its specified value. Perhaps we can do something clever on the DevTools side, otherwise we might need to extend that API.

You can have a fiddle at[1] although it's likely to change and break from time to time. There are lots of other tweaks that need to be made (e.g. highlighting the current property when you hover it to make it obvious why the graph at the top changes), but for now I'd be interested in hearing your overall impression.

[1] https://mozanime.github.io/timewave/

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles; away 14-18 July, busy 19-22 July) from comment #6)
> Hi Patrick, I wonder what you think of attachment 8770434 [details]. There
> are a few ideas there for how this view might look.
> 
> A few points of interest:
> 
> * It starts off with a summary view that is supposed to show the overall
> pattern of the animation and give you a sense for what properties are
> involved. We'd use the same color for the same property (e.g. transform is
> always orange etc.) so you can get an idea at a glance. Colors / opacity
> show the color/opacity in the graph. I'm not sure how we'll distinguish
> between different color-type properties (e.g. if you animate both
> 'background-color' and 'color' at the same time). I'm also not sure how we'd
> distinguish between different types of animations (e.g. transitions vs CSS
> animations) with this view -- perhaps we'd could use the background color.
So, if I understand correctly, this summary view would replace the rectangles you see right now when an animation is displayed in the tool. I think that's a good idea.
This view would also have to represent delay, endDelay and iterationCount as we do today. Also the performance indicator would need to be somewhere there.
In terms of colors, I tend to think that we should stick with the animation type colors we have today, but I can be convinced. As in, you'd have the nice overall pattern, but it would be just one color that corresponds to the animation type (transition, animation, script). And once you'd click on it, then it would colorize the see the various properties involved, as well as expand down, as the screencast shows.

> Once you click the summary view it expands to show the individual properties
> in the same way as the current panel as well as the timing profile. Having a
> summary view probably matters most when shorthands are being used since they
> expand to a lot of subproperties.
> 
> * It tries to represent properties on a natural scale so that an opacity of
> 1 is at the top of the graph and an opacity of 0 is at the bottom. For
> colors, the height is related to the hue (this might need some work in case
> you animate between two colors with different luminance/saturation but with
> a hue of zero). For transforms the idea is we'd expand translateX /
> translateY / scale etc. into separate graphs that are linked together (i.e.
> tweaking one simultaneously updates the others). Where splitting a transform
> out is not possible we'd just fall back to using cumulative distance from
> the start point to represent the height.
I like the idea. This feels pretty natural and looks nice and easily understandable.
When it comes to transforms: see bug 1246198 where we wanted to break transforms into individual components.

> * It includes handles to show how it might be possible to adjust timing
> parameters from this view. It's a bit buggy, and there would need to be more
> space between the graphs so you can drag the handles further, but if this
> approach worked, we'd probably apply similar treatment to the individual
> property graphs too for editing per-keyframe easing / keyframe offsets etc.
> As for switching between different *types* of easing (e.g. between
> cubic-bezier and steps) we might need an extra panel I guess.
Being able to move the timing graph around to change delay, and resize it to change duration seems very natural and an easy thing to do. Adjusting the handles seems harder. First of all we'd have to make sure you can drag anywhere so you're constrained, but that shouldn't be too hard. But ultimately, I wonder if it wouldn't just be easier to have a dedicated panel to do that. It's going to be such a tiny piece of UI to make fine grain adjustments. I expect people want to use this to fine tune their animations. They've come up with something in their text editors, and now they want to test the animation and adjust with tiny increments until they're happy with how the animation runs. And doing this kind of work on the timing graph as proposed in the demo may be hard.
I think people doing animation work in apps like aftereffect might be used to having dedicated (bigger) editors for this kind of things.
Maybe both aren't incompatible btw, handles could be there, as well as a button to open a new panel for more control.

Ultimately, I think we should always have a way for people to enter property values or timing-function text. The tool is only going to go so far as to allow small adjustments. As you said, if the user wants to switch to a step function, or to make a radical property value change for a given keyframe (for which dragging a handle would be hard), then they should be able to do this by entering a value somewhere.
This is basically the job of the rule-view, but if people can do everything in just one panel, it's better. So somehow we need to merge both.
It doesn't mean we have to do this here. To be clear, right now, the animation panel doesn't allow any editing at all. So it'd be totally fine to ship a first version that allows only to use handles to make changes, and require people to switch to the rule-view for more changes (but honestly, a lot of people are going to go back to their text editors to make the change and then reload the page anyway).

> Some other things we're not sure about:
> 
> * I'm not sure if using the same natural scale for the summary view makes
> sense. It means that the height of the summary view doesn't necessarily have
> much corresponding to progress. As a result, you can't clearly see
> animation-direction, or even iterations. Also, when you tweak the timing
> graph, the result is often surprising. I wonder if using cumulative distance
> for the top graph would be better.
I think the very first thing the summary view should be telling users is:
here's an animation, it runs on this element, it has this name, and it lasts this much time.
This may be simple, but the first thing you want to know is whether the CSS you wrote actually was understood by the browser and therefore whether the animation runs, and whether it runs with the right duration, delay, iterations (people may get confused if delay or duration comes first, that kind of stuff). So these things should be very easy to work out just by looking at the summary view.
What do you think of making the timing view the summary view in fact? I guess I need to be convinced of the interest for showing the cumulative properties view first, instead of a graph that only (but clearly) shows the timing components and the way the animation progresses through those.

> * I'm not sure how we should handle shorthands. The getProperties() API
> returns the expanded longhands with computed values but when it comes to
> editing, in some cases it would be easier to edit the property as a
> shorthand and with its specified value. Perhaps we can do something clever
> on the DevTools side, otherwise we might need to extend that API.
Ah, yes, good question. This, somehow, goes back to what I was saying with the rule-view being the current editing tool. It only deals with authored values (so shorthands, if they were used), whereas the animation inspector only deals with computed values.
I would definitely advise shipping first a read-only UI before attempting to solve this issue. As I was saying, the tool is already read-only now, so it wouldn't be a problem to change our current keyframe view with your proposal (actually it'd be great!).
Next we can focus on making properties editable (bug 1211923).
Either we're really smart and we find a way to edit the authored value when dragging the handles on individual properties, or we add an edit mode.
So, one thing we can easily know in devtools is which longhands correspond to which shorthand, so knowing the authored styles (which we do), and the list of longhand animated properties (which we do too), then we could show a UI that merged both.
Say you have an animation for background-position and color, the actual properties that are animated are background-position-x, background-position-y and color.
So in our editing UI we could show something like:

background-position        <- first level, authored, where editing happens
  background-position-x    <- second level, computed, where the graph is shown
  background-position-y    <- second level, computed, where the graph is shown
color                      <- first level, authored, where editing happens

This is very incomplete and abstract, but hopefully you get the idea.
As a user, you'd always only edit the authored values, just like in the rule-view, but you'd see immediately the effect that this has on the various longhands properties, if any.
Of course that would make the drag/drop handle idea impossible, or very hard ... So, just a thought.

> You can have a fiddle at[1] although it's likely to change and break from
> time to time. There are lots of other tweaks that need to be made (e.g.
> highlighting the current property when you hover it to make it obvious why
> the graph at the top changes), but for now I'd be interested in hearing your
> overall impression.
> 
> [1] https://mozanime.github.io/timewave/
Thanks! that helps a lot.

Comment 8

[Brian Birtles (:birtles)]

Attachment: Summary view v1

(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #7)
> (In reply to Brian Birtles (:birtles; away 14-18 July, busy 19-22 July) from
> comment #6)
> > Hi Patrick, I wonder what you think of attachment 8770434 [details]. There
> > are a few ideas there for how this view might look.
> > 
> > A few points of interest:
> > 
> > * It starts off with a summary view that is supposed to show the overall
> > pattern of the animation and give you a sense for what properties are
> > involved. We'd use the same color for the same property (e.g. transform is
> > always orange etc.) so you can get an idea at a glance. Colors / opacity
> > show the color/opacity in the graph. I'm not sure how we'll distinguish
> > between different color-type properties (e.g. if you animate both
> > 'background-color' and 'color' at the same time). I'm also not sure how we'd
> > distinguish between different types of animations (e.g. transitions vs CSS
> > animations) with this view -- perhaps we'd could use the background color.
> So, if I understand correctly, this summary view would replace the
> rectangles you see right now when an animation is displayed in the tool. I
> think that's a good idea.

Yes, that's correct.

> This view would also have to represent delay, endDelay and iterationCount as
> we do today. Also the performance indicator would need to be somewhere there.

Right, we need to add those (although I think iterationCount is already
represented?). I'm not sure how to indicate endDelay just yet.

> In terms of colors, I tend to think that we should stick with the animation
> type colors we have today, but I can be convinced. As in, you'd have the
> nice overall pattern, but it would be just one color that corresponds to the
> animation type (transition, animation, script). And once you'd click on it,
> then it would colorize the see the various properties involved, as well as
> expand down, as the screencast shows.

I also like the current coloring but I think it would be even more useful to be
able to see at a glance which properties are being animated. I believe the
Chrome DevTools gives you that.

For example, if transform was pink and opacity was blue you could very easily,
at a glance, find the animation you're interested in. Also, if you have
multiple transform animations that relationship would be very obvious.

I agree it would be good to have some way to represent the difference types of
animations (CSS Animation / CSS Transition / script animation) though as well.
Perhaps we could use the background color for that?

> > Some other things we're not sure about:
> > 
> > * I'm not sure if using the same natural scale for the summary view makes
> > sense. It means that the height of the summary view doesn't necessarily have
> > much corresponding to progress. As a result, you can't clearly see
> > animation-direction, or even iterations. Also, when you tweak the timing
> > graph, the result is often surprising. I wonder if using cumulative distance
> > for the top graph would be better.
> I think the very first thing the summary view should be telling users is:
> here's an animation, it runs on this element, it has this name, and it lasts
> this much time.

Yes, agreed. I think knowing which properties it affects would also be very
useful (since it helps you filter out the unrelated animations so you don't
need to expand them to see what they affect).

> This may be simple, but the first thing you want to know is whether the CSS
> you wrote actually was understood by the browser and therefore whether the
> animation runs, and whether it runs with the right duration, delay,
> iterations (people may get confused if delay or duration comes first, that
> kind of stuff). So these things should be very easy to work out just by
> looking at the summary view.
> What do you think of making the timing view the summary view in fact? I
> guess I need to be convinced of the interest for showing the cumulative
> properties view first, instead of a graph that only (but clearly) shows the
> timing components and the way the animation progresses through those.

Daisuke and I have made an attempt at doing this. We've taken the timing view
and just overlayed color bands to represent which properties are being animated.
The width of the bands has no meaning at all in this view. Presumably we'd group
different properties under the same color band (e.g. margin-left and margin-top
would be represented by the same band, likewise all background properties). That
also means there are fewer colors to remember. I suppose if we have an animation
that animates 50 different properties, we'd just limit the view to showing 4~5
colors (maybe with some indication that there are more).

We've also tried to represent animation type using a different background color.
For delay the idea is to show a dotted line if the animation doesn't fill
backwards. If the animation fills forwards / backwards we'd just repeat the end
of the shape (possibly fading it out) or use a solid line if the shape is at the
zero point when it fills.

What do you think?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #8)
Sorry Brian for the long de
> I also like the current coloring but I think it would be even more useful to
> be
> able to see at a glance which properties are being animated. I believe the
> Chrome DevTools gives you that.
I'm not sure, I see they have 1 color per animation, but they don't seem to correspond to what properties are being animated. If you animate more than 1 property, it's still just one color. At least that's what I've seen so far, maybe I've missed something.

> For example, if transform was pink and opacity was blue you could very
> easily,
> at a glance, find the animation you're interested in. Also, if you have
> multiple transform animations that relationship would be very obvious.
Good point. If animated properties always have the same colors, then that'd be useful for users. I'm just worried about 2 things: we'd have to make it extra obvious in the UI what the colors are for, and there are so many animatable properties in CSS that the colors might loose meaning (in practice maybe people only ever animate very few properties).

> Daisuke and I have made an attempt at doing this. We've taken the timing view
> and just overlayed color bands to represent which properties are being
> animated.
> The width of the bands has no meaning at all in this view. Presumably we'd
> group
> different properties under the same color band (e.g. margin-left and
> margin-top
> would be represented by the same band, likewise all background properties).
> That
> also means there are fewer colors to remember. I suppose if we have an
> animation
> that animates 50 different properties, we'd just limit the view to showing
> 4~5
> colors (maybe with some indication that there are more).
Definitely in favor of grouping properties by types so we have fewer colors.
I think we need to continue experimenting. Do you have this somewhere online I can look at?
Helen could also help with the final designs, to make sure it looks nice and consistent with the rest of devtools.

> We've also tried to represent animation type using a different background
> color.
If we decide to go with colors for the properties, then maybe we can drop the animation type colors. It might look heavy with so much colors, I think having some white background would help. Plus, animation types can easily be represented by an icon in the sidebar (where the target DOM nodes are) instead. Like JS for script-animations, and CSS for animations and transitions.

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: timing-function.png

Hi Patrich,

I have made a prototype that displays the timing-function on the animation inspector.
I attach the screenshot ( timing-function.png ) and patches.
Could you confirm and give your advice?

Thanks!

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: 1210795-timingfunction-1.patch

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: 1210795-timingfunction-2.patch

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Daisuke Akatsuka (:daisuke) from comment #10)
> Created attachment 8784255 [details]
> timing-function.png
> 
> Hi Patrich,
> 
> I have made a prototype that displays the timing-function on the animation
> inspector.
> I attach the screenshot ( timing-function.png ) and patches.
> Could you confirm and give your advice?
> 
> Thanks!
Sure! Thanks for this. Should I apply both patches?
Setting NI? so I remember to take a look at this (don't hesitate to set F? on patches you'd like me to try).

Comment 14

[Brian Birtles (:birtles)]

Attachment: Delays and fill modes proposal

I had a bit of a think about how to represent delays and fill modes and this is what I came up with.

I'm not quite sure how this will work with negative start delays because I'm not even sure if they will be visible in the graph. Will they?

Comment 15

[Brian Birtles (:birtles)]

Questions/comments for Patrick:

* It looks like the animation inspector doesn't currently use React/Redux. Should Daisuke first rewrite the existing graph panel in React/Redux before he reworks it? He's currently using canvas to draw the graph--hopefully that's ok.

* Regarding having colors corresponding to specific properties, I'm not sure what Chrome currently does, but in this video of their upcoming changes it looks like they consistently color opacity as grey and transform as purply-pink: https://www.youtube.com/watch?v=U9xfYbKxosI (see, e.g. 0:43). I notice they also use grey for 'color' so maybe there's some coding going on there.

* The ideas outlined in bug 8784685 don't include the per-property color segments. Perhaps we can do that as a separate step/bug.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8784257 [details] [diff] [review]
1210795-timingfunction-2.patch

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

This looks good, thanks.
It would be great if you could add some code to test this in devtools\client\animationinspector\test\browser_animation_timeline_shows_time_info.js
I'm still looking at part 1. Will probably be done with it early next week.

Comment 17

[Daisuke Akatsuka (:daisuke)]

Thank you Patrick!

Well, I have modified both patch to see the visual that Brian suggested.
I'll upload the patches to moz review board.

Also, I'll append the test code too.
Thanks!

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8784256 [details] [diff] [review]
1210795-timingfunction-1.patch

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

Just a few fly-by comments on this patch, since you're uploading a new one soon anyway.

::: devtools/client/animationinspector/components/animation-time-block.js
@@ +63,3 @@
>        attributes: {
>          "class": "iterations" + (state.iterationCount ? "" : " infinite"),
> +        "width": 300,

Why 300px? The canvas will be resized when the panel resizes, right? Shouldn't the size of the canvas be changed when we resize the panel, and the content re-drawn?

@@ +70,5 @@
> +    const resolutionX = canvas.width;
> +    const resolutionY = canvas.height;
> +    const negativeDelay = state.delay < 0 ? state.delay : 0;
> +    const promise =
> +      animation.getProgresses(

You added a trait in animation-controller.js that's useful to know if the server you are connected to actually supports the getProgresses method. If it doesn't, then you should fallback to a rectangle, or something, but we do need a fallback. With devtools, you can easily debug something remote and in which case, you don't know if that something actually has the getProgresses method.
So you should wrap this in something like AnimationsController.traits.hasGetProgresses

@@ +75,5 @@
> +        negativeDelay, TimeScale.getDuration() + negativeDelay, resolutionX);
> +    const win = this.containerEl.ownerDocument.defaultView;
> +    context.strokeStyle = win.getComputedStyle(canvas).color;
> +    promise.then(progresses => {
> +      for (let x = 0; x < resolutionX; x++) {

I'm a bit worried with the approach taken to draw the canvas.
The timeline is often better seen when the panel is big, and we want to eventually find a new place in the UI to contain it so users don't have to always resize the sidebar to see animations easily.
So, it's possible that the canvas will be something around 1000px wide, and it's probable that you'll be having more than 1 animation displayed at the same time.
That means that everytime the panel refreshes (which is rather often, since we do it on play/pause/rewind/animation events), then we'll get several arrays of data with 1000 entries in them.
When debugging animations locally in firefox, this will probably never be felt as a perf problem, but when debugging remotely, it could be more of a problem.

Also perf related: on the serve (in the AnimationPlayerActor code), could there be perf problems with iterating over the resolution, 1 by 1, and getting the progress from the Animation?

Are there other solutions than this one? If not, would it work to send a lower resolution but then smooth the curve in canvas later?

Finally, also perf related: I noticed when pressing play/pause several times, that every time the canvas flickers a little bit when it gets redrawn. We'll want to investigate why and see if we can have an immediate refresh instead.

Comment 19

[Patrick Brosset <:pbro>]

Oh and another couple of comments about the part 1 patch:
- the name of the animation isn't visible anymore in most cases. Say you have a linear timing function, then that ends up displaying a triangle for the animation, and therefore the animation name is often displayed over a part where the triangle isn't, so it's essentially white text over white background
- I tried messing around with custom cuboic-bezier curves and different easing functions per keyframes, but I never managed to have anything else than a linear function displayed in the timeline. It looked to me like getEasing always returned linear.

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #14)
> Created attachment 8784685 [details]
> Delays and fill modes proposal
> 
> I had a bit of a think about how to represent delays and fill modes and this
> is what I came up with.
> 
> I'm not quite sure how this will work with negative start delays because I'm
> not even sure if they will be visible in the graph. Will they?
I like this a lot, very nice and clean. Most of the first use cases are pretty easy to understand. Of course people aren't going to know at first what a gray and green delay lines mean, but hopefully the tooltip will be enough for them to understand the mapping.
It gets really complex when you have overlapping start/end delays, but really, I'm not sure how common this would be in the wild, and if any representation at all would be clearer anyway.
The only thing I can think of is slightly offsetting the start and end delays on the Y axis, so they never overlap. But again, if it's really rare that they do in practice, then it would look weird for the other 99% of cases.

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #15)
> Questions/comments for Patrick:
> 
> * It looks like the animation inspector doesn't currently use React/Redux.
> Should Daisuke first rewrite the existing graph panel in React/Redux before
> he reworks it? He's currently using canvas to draw the graph--hopefully
> that's ok.
Using canvas is OK, whatever makes it easy to draw what we need.
I'm really for a React/Redux refactor on the long run, because that's one of our team goal, but I wouldn't do it now. The change you're making here is quite local to how each animation is drawn, whereas moving the React and Redux would mean an entire refactor of the panel as a whole. It'd be more work for no visible benefits to the user, and more importantly, we're currently in a refactoring phase already at the moment with the devtools.html project (getting rid of all our XUL and chrome-privileged code), so it would be risky to do this at the same time.
> * Regarding having colors corresponding to specific properties, I'm not sure
> what Chrome currently does, but in this video of their upcoming changes it
> looks like they consistently color opacity as grey and transform as
> purply-pink: https://www.youtube.com/watch?v=U9xfYbKxosI (see, e.g. 0:43). I
> notice they also use grey for 'color' so maybe there's some coding going on
> there.
That's a 1.5 year old video, so they might have changed things around since then.
I tested again in canary 54.0.2839.2 and I don't understand their colors. Most of the animations I see are gray, some are blue. And for property transitions, sometimes I see margin and transform brown, background positions dark and light blue, even though they are the same property.
> * The ideas outlined in bug 8784685 don't include the per-property color
> segments. Perhaps we can do that as a separate step/bug.
(you mean in attachment 8784685 [details]). Yeah, I'm all for doing this as a separate bug. It will be complex enough already to get the right actor methods and curves displayed in the UI, so let's get that in first.

Comment 22

[Brian Birtles (:birtles)]

(In reply to Patrick Brosset <:pbro> from comment #18)
> @@ +75,5 @@
> > +        negativeDelay, TimeScale.getDuration() + negativeDelay, resolutionX);
> > +    const win = this.containerEl.ownerDocument.defaultView;
> > +    context.strokeStyle = win.getComputedStyle(canvas).color;
> > +    promise.then(progresses => {
> > +      for (let x = 0; x < resolutionX; x++) {
> 
> I'm a bit worried with the approach taken to draw the canvas.
> The timeline is often better seen when the panel is big, and we want to
> eventually find a new place in the UI to contain it so users don't have to
> always resize the sidebar to see animations easily.
> So, it's possible that the canvas will be something around 1000px wide, and
> it's probable that you'll be having more than 1 animation displayed at the
> same time.
> That means that everytime the panel refreshes (which is rather often, since
> we do it on play/pause/rewind/animation events), then we'll get several
> arrays of data with 1000 entries in them.
> When debugging animations locally in firefox, this will probably never be
> felt as a perf problem, but when debugging remotely, it could be more of a
> problem.
> 
> Also perf related: on the serve (in the AnimationPlayerActor code), could
> there be perf problems with iterating over the resolution, 1 by 1, and
> getting the progress from the Animation?
> 
> Are there other solutions than this one? If not, would it work to send a
> lower resolution but then smooth the curve in canvas later?

I spoke to Daisuke about this and we agree we need another approach here. I think we can sample at a much lower frequency and smooth out the curve (e.g. using a Catmull-Rom curve). The one thing we need to be careful of, however, is discrete jumps. However, we generally know where they are: at iteration boundaries and at steps in discrete timing functions, both of which we can calculate in advance. So, if we simply add extra samples either side of these discrete boundary and choose an appropriate smoothing function, I think we can reproduce the curve accurately with much fewer samples.

Using fewer samples and constructing a path (using either canvas or SVG) also means we can stroke it.

In terms of reducing network bandwidth, we could possibly just pass the timing parameters and then "rehydrate" the animation on the client. In fact, all we really need to do is `new Animation(new KeyframeEffect(null, null, timing), null)`. By having a null timeline and null target effect, sampling the animation might be a little faster since we'll skip doing any unnecessary notifications. (And, on that note, we really should be sampling a clone of the animation anyway since seeking the original animation can have side effects like resolving promises etc.)

(In reply to Patrick Brosset <:pbro> from comment #19)
> Oh and another couple of comments about the part 1 patch:
> - the name of the animation isn't visible anymore in most cases. Say you
> have a linear timing function, then that ends up displaying a triangle for
> the animation, and therefore the animation name is often displayed over a
> part where the triangle isn't, so it's essentially white text over white
> background

That's a good point. Perhaps, like the compositor icon, we could use a darker shade of green/yellow/blue so that if shows up regardless of whether it is on the white background or not?

> - I tried messing around with custom cuboic-bezier curves and different
> easing functions per keyframes, but I never managed to have anything else
> than a linear function displayed in the timeline. It looked to me like
> getEasing always returned linear.

Yes, I realized this last week too. As you're probably aware, there are two levels of easing: effect-level easing and keyframe-level easing. As a result, it's possible for different properties to follow different timing functions. In the original timewave proposal[1] showed the composite of *both* effect-level easing and keyframe-level easing in the summary view and then when you expanded it, you saw effect-level easing/timing at the top, then keyframe-level easing for each property.

Somewhere along the way, we decided to make the summary view just show the effect-level easing/timing. For CSS animations, however, animation-timing-function maps to keyframe-level easing so it's not reflected in the summary view.

Some of the things we could do here:

* Map transition-timing-function to *effect*-level easing, that way at least you get curves for transitions. (For script-generated animations, effect-level easing is more common so you should see curves for those.)

* Work on displaying keyframe-level easing in the expanded view so that we can land it at roughly the same time so that at least when you *expand* a CSS animation you see curves.

* Alternatively, we could do something more like the original prototype and overlay the curves for different properties in the summary view. For 99% of animations the curves will match so it won't look more complicated.

[1] https://people.mozilla.org/~bbirtles/devtools/graph.html

(In reply to Patrick Brosset <:pbro> from comment #20)
> (In reply to Brian Birtles (:birtles) from comment #14)
> > Created attachment 8784685 [details]
> > Delays and fill modes proposal
> > 
> > I had a bit of a think about how to represent delays and fill modes and this
> > is what I came up with.
> > 
> > I'm not quite sure how this will work with negative start delays because I'm
> > not even sure if they will be visible in the graph. Will they?
> I like this a lot, very nice and clean. Most of the first use cases are
> pretty easy to understand. Of course people aren't going to know at first
> what a gray and green delay lines mean, but hopefully the tooltip will be
> enough for them to understand the mapping.
> It gets really complex when you have overlapping start/end delays, but
> really, I'm not sure how common this would be in the wild, and if any
> representation at all would be clearer anyway.
> The only thing I can think of is slightly offsetting the start and end
> delays on the Y axis, so they never overlap. But again, if it's really rare
> that they do in practice, then it would look weird for the other 99% of
> cases.

Yes, I agree. I think these cases are so rare that it probably doesn't matter. Also, when we make these delays editable, I imagine that when you hover over one, it will switch from being semi-transparent to fully opaque. That should make it possible to distinguish between the two lines in the rare case where they overlap (e.g. hover of the left end and the start delay line becomes opaque show you can distinguish it from the end delay.)

(In reply to Patrick Brosset <:pbro> from comment #21)
> (In reply to Brian Birtles (:birtles) from comment #15)
> > Questions/comments for Patrick:
> > 
> > * It looks like the animation inspector doesn't currently use React/Redux.
> > Should Daisuke first rewrite the existing graph panel in React/Redux before
> > he reworks it? He's currently using canvas to draw the graph--hopefully
> > that's ok.
> Using canvas is OK, whatever makes it easy to draw what we need.
> I'm really for a React/Redux refactor on the long run, because that's one of
> our team goal, but I wouldn't do it now. The change you're making here is
> quite local to how each animation is drawn, whereas moving the React and
> Redux would mean an entire refactor of the panel as a whole. It'd be more
> work for no visible benefits to the user, and more importantly, we're
> currently in a refactoring phase already at the moment with the
> devtools.html project (getting rid of all our XUL and chrome-privileged
> code), so it would be risky to do this at the same time.

Oh, that's good to know, thanks!

> > * Regarding having colors corresponding to specific properties, I'm not sure
> > what Chrome currently does, but in this video of their upcoming changes it
> > looks like they consistently color opacity as grey and transform as
> > purply-pink: https://www.youtube.com/watch?v=U9xfYbKxosI (see, e.g. 0:43). I
> > notice they also use grey for 'color' so maybe there's some coding going on
> > there.
> That's a 1.5 year old video, so they might have changed things around since
> then.
> I tested again in canary 54.0.2839.2 and I don't understand their colors.
> Most of the animations I see are gray, some are blue. And for property
> transitions, sometimes I see margin and transform brown, background
> positions dark and light blue, even though they are the same property.

Oh interesting. I don't know what that is then--using the same color would be so useful!

Comment 23

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review commit: https://reviewboard.mozilla.org/r/74490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74490/

Comment 24

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 2: Changes Infinity expression.

Review commit: https://reviewboard.mozilla.org/r/74492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74492/

Comment 25

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review commit: https://reviewboard.mozilla.org/r/77336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77336/

Comment 26

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review commit: https://reviewboard.mozilla.org/r/77338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77338/

Comment 27

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 5: Updates popup information.

Review commit: https://reviewboard.mozilla.org/r/77340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77340/

Comment 28

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 6: Removes extra codes.

Review commit: https://reviewboard.mozilla.org/r/77342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77342/

Comment 29

[Daisuke Akatsuka (:daisuke)]

Hi Patrick,

I wrote the patches. 
Couldn't you review?

I did not add any tests for this yet.
But if basic algorithm and structure are ok, I'll make tests. 

Thanks!

Comment 30

[Daisuke Akatsuka (:daisuke)]

Also, I attach the screen shots.
Please refer this too.
https://docs.google.com/a/mozilla.com/presentation/d/1dwmvUOLvkjJdvdv5xtD0LUM0WFACX6uYyPRXhuE8Zsw/edit?usp=sharing

Comment 31

[Daisuke Akatsuka (:daisuke)]

Also, some results are different from attachment 8784685 [details] (e.g. fade effect).
I'll implement after the first review.

Thanks!

Comment 32

[Patrick Brosset <:pbro>]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

https://reviewboard.mozilla.org/r/77340/#review76306

Comment 33

[Patrick Brosset <:pbro>]

Sorry for the delay on reviewing these patches. I have started end of last week, and will finish this week. I'm at a conference Monday, Tuesday and Wednesday however, so won't be able to review until Thursday.

Comment 34

[Daisuke Akatsuka (:daisuke)]

Okay, thanks!

Comment 35

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/1-2/

Comment 36

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/1-2/

Comment 37

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/1-2/

Comment 38

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/1-2/

Comment 39

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/1-2/

Comment 40

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/1-2/

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

https://reviewboard.mozilla.org/r/74490/#review77516

Nice work. I like how you now use a local Animation object to draw the curve instead of requiring this data from the server. Nice!

I have a couple of general comments first:
- an easing like `cubic-bezier(.68,-0.55,.27,1.55)` means that the start and end section are going to be cropped out. I think that's fine, I don't see how we could do otherwise without risking to hide other parts of the UI, but I just want to make sure we are conscious about this,
- I'm not sure I completely understand comment 22 (about effect-level easing and keyframe-level easing). I was able to see the nice easing curves on script-animations, but not for CSS animations. comment 22 says that for CSS animations, the animation-timing-function maps to keyframe-level easing. I'm not sure I understand that part. As a user, I guess I would be expecting `animation-timing-function: ease-in-out;` to cause a nice curve to appear in the animation-editor, just like we do for effect-level easing in script-animations. Could we discuss this some more? Could we somehow represent animation-timing-function here?

::: devtools/client/animationinspector/components/animation-time-block.js:60
(Diff revision 2)
>        TimeScale.getAnimationDimensions(animation);
>  
>      // background properties for .iterations element
>      let backgroundIterations = TimeScale.getIterationsBackgroundData(animation);
>  
> -    createNode({
> +    // displayed total duation

s/duation/duration

::: devtools/client/animationinspector/components/animation-time-block.js:63
(Diff revision 2)
>      let backgroundIterations = TimeScale.getIterationsBackgroundData(animation);
>  
> -    createNode({
> +    // displayed total duation
> +    const totalDuration = TimeScale.getDuration();
> +
> +    // Animation summary graph --------

Please remove the `-------------` part of this comment. 
Usually separators in comments are a sign that you should split the code up in several functions.
See my comment below about this.

::: devtools/client/animationinspector/components/animation-time-block.js:65
(Diff revision 2)
> +    const timing = {
> +      duration: state.duration,
> +      delay: state.delay,
> +      endDelay: state.endDelay,
> +      easing: state.easing,
> +      direction: state.direction,
> +      fill: state.fill,
> +      iterations: state.iterationCount ? state.iterationCount : 1,
> +      iterationStart: state.iterationStart,
> +    };

This can be done with fewer lines of code:

```
const timing = Object.assign({}, state, {
  iterations: state.iterationCount ? state.iterationCount : "Infinity"
});
```

::: devtools/client/animationinspector/components/animation-time-block.js:65
(Diff revision 2)
> +    const timing = {
> +      duration: state.duration,
> +      delay: state.delay,
> +      endDelay: state.endDelay,
> +      easing: state.easing,
> +      direction: state.direction,
> +      fill: state.fill,
> +      iterations: state.iterationCount ? state.iterationCount : 1,
> +      iterationStart: state.iterationStart,
> +    };
> +    const win = this.containerEl.ownerDocument.defaultView;
> +    const webAnimation =
> +      new win.Animation(new win.KeyframeEffect(null, null, timing), null);
> +
> +    // function to get path segment by time
> +    const getPathSegment = time => {
> +      webAnimation.currentTime =
> +        time * state.playbackRate + Math.min(timing.delay, 0);
> +      const progress = webAnimation.effect.getComputedTiming().progress;
> +      return { x: time, y: Math.max(progress, 0) };
> +    };

I would move all of this code into a simple function at the botom of the file. This would make the render function simpler, and easier to understand.
Nothing here depends on `this`, so it's a good candidate for something to be moved outside of the prototype of this class:

```
function getSegmentHelper({state}, win) {
  // Create a dummy Animation with the same timing data as the 
  // animation object we're being passed in.
  const timing = Object.assign({}, state, {
    iterations: state.iterationCount ? state.iterationCount : "Infinity"
  });
  const dummyAnimation = new win.Animation(new win.KeyframeEffect(null, null, timing), null);

  // Return a helper function that, given a time, will calculate the progress through the
  // dummy animation.
  return time => {
    dummyAnimation.currentTime = time * state.playbackRate + Math.min(timing.delay, 0);
    const progress = dummyAnimation.effect.getComputedTiming().progress;
    return { x: time, y: Math.max(progress, 0) };
  };
}
```

This way, inside the `render` method, you can do:

```
const getSegment = getSegmentHelper(animation, this.win);
```

Notice that I've used `this.win`, for this to work, you could simply define a getter method before the render method:

```
get win() {
  return this.containerEl.ownerDocument.defaultView;
}
```

::: devtools/client/animationinspector/components/animation-time-block.js:91
(Diff revision 2)
> +
> +    const pathSegments = [];
> +    // Append path segments of delay
> +    if (delayW) {
> +      const segments = this.createPathSegments(0, timing.delay, getPathSegment);
> +      Array.prototype.push.apply(pathSegments, segments);

I'm not a huge fan of using `apply` here. If I'm not mistaken you want to concatenate arrays here.
Why not simply do:

`pathSegments = pathSegments.concat(segments);`

Same comment for the 4 other places where you've used `apply` below.

::: devtools/client/animationinspector/components/animation-time-block.js:95
(Diff revision 2)
> +      const segments = this.createPathSegments(0, timing.delay, getPathSegment);
> +      Array.prototype.push.apply(pathSegments, segments);
> +    }
> +
> +    // Append path segments of iterations
> +    {

Why a nested block here?

::: devtools/client/animationinspector/components/animation-time-block.js:106
(Diff revision 2)
> +          this.createPathSegments(startTime, endTime, getPathSegment);
> +        Array.prototype.push.apply(pathSegments, segments);
> +      }
> +      const fraction = timing.iterations - iterationCount;
> +      if (fraction) {
> +        const startTime = delay + iterationCount * timing.duration;

ESLint tells me that `delay` isn't defined here, and indeed, it isn't.

::: devtools/client/animationinspector/components/animation-time-block.js:107
(Diff revision 2)
> +        Array.prototype.push.apply(pathSegments, segments);
> +      }
> +      const fraction = timing.iterations - iterationCount;
> +      if (fraction) {
> +        const startTime = delay + iterationCount * timing.duration;
> +        const endTimeRate = startTime + fraction * timing.duration;

`endTimeRate` isn't used anywhere in this scope. I think you meant `endTime` instead, since you use it on the next line but it's not defined.

::: devtools/client/animationinspector/components/animation-time-block.js:198
(Diff revision 2)
>          }
>        });
>      }
>    },
>  
> +  createPathSegments: function (startTime, endTime, getPathSegment) {

This function doesn't need access to anything on the instance (i.e. it doesn't use `this` other than to call itself). So I suggest moving it as a simple function at the end of the file (outside of the prototype), so it doesn't clutter the class itself.

Also, please add a jsdoc comment before it, so future readers know what it's for and what the parameters and return value should be.

::: devtools/client/animationinspector/components/animation-time-block.js:199
(Diff revision 2)
>        });
>      }
>    },
>  
> +  createPathSegments: function (startTime, endTime, getPathSegment) {
> +    const RESOLUTION = 4;

Please move this constant to the top of the file, with a preceeding comment that explains what this number is for.

::: devtools/client/animationinspector/components/animation-time-block.js:200
(Diff revision 2)
>      }
>    },
>  
> +  createPathSegments: function (startTime, endTime, getPathSegment) {
> +    const RESOLUTION = 4;
> +    if (endTime - startTime < 10) {

10 is a magic number here, please define it as a constant (with a comment) at the top of the file instead.

::: devtools/client/animationinspector/components/animation-time-block.js:200
(Diff revision 2)
> +    if (endTime - startTime < 10) {
> +      const segment = getPathSegment((startTime + endTime) / 2);
> +      return [segment];
> +    }

One general comment I have about this function is the lack of comments and empty lines. It makes it harder to read.
So, in particular, for this `if` block. I would separate it from the rest with empty lines and preceed it with a comment:

```

// Early return in case the duration is too short.
if (endTime - startTime < MIN_SEGMENT_DURATION) {
  return [getPathSegment((startTime + endTime) / 2)];
}

```

This way it stands out more, and is very obvious what its role is.
So readers can directly skip it and read the rest of the function which is more important.

Also, why `startTime+endTime/2`? Why not just endTime? I'm not sure I understand the logic of taking the mid point here.

::: devtools/client/animationinspector/components/animation-time-block.js:204
(Diff revision 2)
> +    const RESOLUTION = 4;
> +    if (endTime - startTime < 10) {
> +      const segment = getPathSegment((startTime + endTime) / 2);
> +      return [segment];
> +    }
> +    const pathSegments = [];

Similarly, I would add a comment before this line that explains what the whole block after is supposed to be doing. Something like:

```
// Otherwise, create segments for ...
```

::: devtools/client/animationinspector/components/animation-time-block.js:205
(Diff revision 2)
> +    const timeTick = (endTime - startTime) / RESOLUTION;
> +    const startSegment = getPathSegment(startTime);
> +    pathSegments.push(startSegment);
> +    let previousSegment = startSegment;
> +    for (let index = 1; index <= RESOLUTION; index++) {
> +      const time = startTime + index * timeTick;
> +      const segment = getPathSegment(time);
> +      if (Math.abs(segment.y - previousSegment.y) > 0.01) {
> +        const subStartTime = previousSegment.x + 1; // 1ms delay
> +        const subEndTime = segment.x - 1;
> +        const subPathSegments =
> +          this.createPathSegments(subStartTime,
> +                                  subEndTime, getPathSegment);
> +        Array.prototype.push.apply(pathSegments, subPathSegments);
> +      }
> +      pathSegments.push(segment);
> +      previousSegment = segment;
> +    }
> +    return pathSegments;

It took me a while to understand the logic here. It's the most important part of this patch and it should be easier to follow.
Adding comments would be nice, to explain the logic.

Also I noticed one thing: whether the sidebar is very wide or very narrow, that has no impact at all on the logic here. So it means that we're making equally complex paths whether we have space to draw them or not.

Could this have SVG performance implications? The `d` attribute right now contains several hundreds of path commands.

If we took the panel width into consideration to simplify the path, then that means we'd also need to refresh it whenever the panel width changes. So maybe it's not worth it. But I'm just worried by the amount of data in the path.

I'll have a look at this code in more details at the next review, but there are already a few comments here that I think I'll publish this one now.

::: devtools/client/animationinspector/utils.js:30
(Diff revision 2)
>   * - nodeType {String} Optional, defaults to "div",
>   * - attributes {Object} Optional attributes object like
>   *   {attrName1:value1, attrName2: value2, ...}
>   * - parent {DOMNode} Mandatory node to append the newly created node to.
>   * - textContent {String} Optional text for the node.
> + * - xmlns {String} Optional namespace

I think you could name this parameter `namespace` instead.

::: devtools/client/themes/animationinspector.css:361
(Diff revision 2)
> +  stroke: var(--timeline-border-color);
> +}
> +
>  /* Animation iterations */
>  
>  .animation-timeline .animation .iterations {

This rule, and several other rules below it aren't needed anymore. You have removed the div.iterations from the UI, so the corresponding CSS should be removed too.

::: devtools/server/actors/animation.js:242
(Diff revision 2)
>    getIterationStart: function () {
>      return this.player.effect.getComputedTiming().iterationStart;
>    },
>  
> +  /**
> +   * Get the animation easing from this player, in text.

I think the ", in text" part of this comment is unnecessary, because you already have a `@return {String}` jsdoc below.

::: devtools/server/actors/animation.js:250
(Diff revision 2)
> +  getEasing: function () {
> +    return this.player.effect.timing.easing;
> +  },
> +
> +  /**
> +   * Get the animation fill mode from this player, in text.

same here

::: devtools/server/actors/animation.js:258
(Diff revision 2)
> +  getFill: function () {
> +    return this.player.effect.getComputedTiming().fill;
> +  },
> +
> +  /**
> +   * Get the animation direction from this player, in text.

and here

::: devtools/server/actors/animation.js:307
(Diff revision 2)
> +      fill: this.getFill(),
> +      easing: this.getEasing(),
> +      direction: this.getDirection(),

Can you please modify test:
devtools\server\tests\browser\browser_animation_playerState.js
based on this?
This test should make sure all animation actors do return the expected properties in their state objects.

Comment 42

[Brian Birtles (:birtles)]

(In reply to Patrick Brosset <:pbro> from comment #41)
> - I'm not sure I completely understand comment 22 (about effect-level easing
> and keyframe-level easing). I was able to see the nice easing curves on
> script-animations, but not for CSS animations. comment 22 says that for CSS
> animations, the animation-timing-function maps to keyframe-level easing. I'm
> not sure I understand that part. As a user, I guess I would be expecting
> `animation-timing-function: ease-in-out;` to cause a nice curve to appear in
> the animation-editor, just like we do for effect-level easing in
> script-animations. Could we discuss this some more? Could we somehow
> represent animation-timing-function here?

Conceptually, there are at least two different ways to apply easing:

(a) Across a the whole iteration of an animation, affecting all properties alike [ effect easing ]
(b) Between keyframes and only affecting the properties specified on those keyframes [ keyframe easing ]

CSS transitions has only a single property and single keyframe-pair so (a) and (b) are identical.

CSS animations only does (b) and this confuses and frustrates a lot of people. Nearly everyone assumes that animation-timing-function does (a) but it does (b). What that means is that you can have different easing applied to different properties within a single animation.

The Web Animations' API offers both (a) and (b) but, by necessity, maps animation-timing-function to (b). For transition-timing-function we currently map it to (b) but could easily map it to (a).

For visualizing (b), given that different properties can have different easing (or the same easing but applied to different offsets), we can do a few different things including at least:

1. Draw overlapping curves (as in the original proposal: [1]) -- recognizing that ~99% of the time these curves will not differ
2. Draw just (a) in the summary view, but then when you expand the summary view to see individual properties, show (b) there (which is what we want to do anyway, and which is what Daisuke is working on in bug 1210796).

[1] http://codepen.io/birtles/pen/OXVebR

Comment 43

[Daisuke Akatsuka (:daisuke)]

Thanks, Patrich!
I'll fix them.

Sorry, this is not related to this bug, but I have one question about development environment.
I’m developing using "Developer Tools (local version)”. However, I could not reboot the devtools correctly. The error message is below.
JavaScript error: resource://devtools/shared/Loader.jsm, line 74: Error: Module `acorn/acorn` is not found at resource://devtools/acorn/acorn.js
It works well, if I changed the following paths.

- resource://devtools/acorn > resource://devtools/shared/acorn
- resource://devtools/acorn/walk.js > resource://devtools/shared/acorn/walk.js

But I got an error if I boot using './mach build' with these paths.
JavaScript error: resource://devtools/shared/Loader.jsm, line 74: Error: Module `acorn/acorn` is not found at resource://devtools/shared/acorn/acorn.js

Are there the same situations in other environment?
# I could not find such the bugs..
My environment: OSX 10.10.5 .

Comment 44

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #42)
> Conceptually, there are at least two different ways to apply easing:
> 
> (a) Across a the whole iteration of an animation, affecting all properties
> alike [ effect easing ]
> (b) Between keyframes and only affecting the properties specified on those
> keyframes [ keyframe easing ]
> 
> CSS transitions has only a single property and single keyframe-pair so (a)
> and (b) are identical.
> 
> CSS animations only does (b) and this confuses and frustrates a lot of
> people. Nearly everyone assumes that animation-timing-function does (a) but
> it does (b). What that means is that you can have different easing applied
> to different properties within a single animation.
Thanks a lot Brian for taking the time to explain this again. That helps a lot.

> The Web Animations' API offers both (a) and (b) but, by necessity, maps
> animation-timing-function to (b). For transition-timing-function we
> currently map it to (b) but could easily map it to (a).
> 
> For visualizing (b), given that different properties can have different
> easing (or the same easing but applied to different offsets), we can do a
> few different things including at least:
> 
> 1. Draw overlapping curves (as in the original proposal: [1]) -- recognizing
> that ~99% of the time these curves will not differ
Ok, I get it now. I think it does make sense then to do this because:
- indeed, 99% of the time, this will look just like one curve only,
- when several curves are drawn, then that's a good thing for the user who will (like me just now) understand that animation-timing-function maps to (b)
I don't think we should do this just now thought. This bug is already complex, and Daisuke is doing great work that I wouldn't want to interrupt. So let's go on with the current approach and file an other bug for representing keyframe timing functions in the summary view when the animation is either a CSS animation or a CSS transition.

Comment 45

[Patrick Brosset <:pbro>]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

https://reviewboard.mozilla.org/r/74492/#review77762

As commented below, I think we need to test this more in cases where there are many animations being displayed at the same time.
https://dl.dropboxusercontent.com/u/714210/iterations.html should help fix some of the problems described below.

::: devtools/client/animationinspector/components/animation-time-block.js:72
(Diff revision 2)
>        delay: state.delay,
>        endDelay: state.endDelay,
>        easing: state.easing,
>        direction: state.direction,
>        fill: state.fill,
> -      iterations: state.iterationCount ? state.iterationCount : 1,
> +      iterations: state.iterationCount ? state.iterationCount : "Infinity",

Shouldn't this be `Infinity` instead of `"Infinity"`? Maybe both are valid.

::: devtools/client/animationinspector/components/animation-time-block.js:158
(Diff revision 2)
> +      const infinityElement = createNode({
> +        parent: this.containerEl,
> +        xmlns: "http://www.w3.org/2000/svg",
> +        nodeType: "svg",
> +        attributes: {
> +          "class": "summary infinity",
> +          "preserveAspectRatio": "none"
> +        }
> +      });

Do we really need a second SVG element?
I'm wondering if we couldn't just use the .summary SVG element instead. 
Using the TimeScale helper, we know just how long is the longest animation displayed in the graph, so for an infinite animation, it would be easy to know how many iterations of it we should drawn to fill in the whole width of the graph.
This way we'd need to create only one SVG element and one path.

::: devtools/client/animationinspector/utils.js:147
(Diff revision 2)
>           iterationCount, playbackRate} = state;
>  
>      endDelay = typeof endDelay === "undefined" ? 0 : endDelay;
>      let toRate = v => v / playbackRate;
>      let minZero = v => Math.max(v, 0);
> -    let rateRelativeDuration =
> +    this.displayedIterationCount = !iterationCount ? 2 : iterationCount;

I guess the reasoning behind the number 2 here is that if an animation repeats infinitely, then you want to display 2 iterations so that users see that it actually repeats, right?

The number 2 should be a constant at the top of the file with a self-explanatory name and a comment:

```
// Show 2 iterations for infinite animations to give users a clue that the animation does repeat.
const INFINITE_ANIMATIONS_ITERATIONS = 2;
```

I think there's another problem here. The TimeScale helper is something we use to know what's the total duration of the longest animation displayed in the tool. This way we can use it to know how long should a given animation be, with respect to the longest (everything in the panel is in CSS %, the longest animation is 100%, and all the other ones are sized as % of it).

Which means that when we refresh the panel, we call `addAnimation` several times, once per animation.
So with the change you made, `this.displayedIterationCount` will be overridden every time. And `getDisplayedIterationCount` will just return the last value that happened to have been set.

If you always have just 1 animation displayed, that's fine, you're probably not seeing any problem.
Try to display several animations at once, mixing different iteration-counts, and you'll probably start seeing issues.

In fact, here's an example:
https://dl.dropboxusercontent.com/u/714210/iterations.html
- open this page
- open the inspector and leave the <body> node selected
- look at the animation panel

You should see that that causes problems:
- the first animation for instance only repeats once, and then fills forward. But you can see that after 5 iterations' worth of time, the forward fill goes down linearly to 0.
- also, the infinite aniation only seems to repeat 5 times, and then there's a flat line until the end of the graph where the infinite path is shown.

Comment 46

[Patrick Brosset <:pbro>]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

https://reviewboard.mozilla.org/r/77342/#review77766

Looks good, thanks for the cleanup.

Comment 47

[Patrick Brosset <:pbro>]

I'll let you take the time to address my comments in parts 1 and 2 and will then look at the other parts that I haven't reviewed yet. I believe there's enough work to be done already before I dig in further.

Just wanted to say that this is very exciting, although there is still work to do, this is going in the right direction, and is looking pretty great! I really like how that gives users much more information at a glance than before.

Comment 48

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/2-3/

Comment 49

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/2-3/

Comment 50

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/2-3/

Comment 51

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/2-3/

Comment 52

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/2-3/

Comment 53

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/2-3/

Comment 54

[Daisuke Akatsuka (:daisuke)]

Attachment: 20160922-screenshot.png

Hi Patrich,
Thank you for your review and very kindly feedback!

I modified the patches and the representation of Infinity iterations and fill-mode. Please confirm and review it.
I attached the new screenshot.

I'll add tests for the summary graph later.

Thanks!

Comment 55

[Daisuke Akatsuka (:daisuke)]

Attachment: 20160922-screenshot-alternate.png

Also, attached a screenshot of "alternate" direction.

Comment 56

[Daisuke Akatsuka (:daisuke)]

Hi Patrich,
I am adding tests of summary graph to patch 1, 2 now.
Also modify calculation logic a bit.
So, please wait to review.

Comment 57

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d2d9468518

Comment 58

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/3-4/

Comment 59

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/3-4/

Comment 60

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/3-4/

Comment 61

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/3-4/

Comment 62

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/3-4/

Comment 63

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/3-4/

Comment 64

[Brian Birtles (:birtles)]

Just a few drive-by comments:

* It looks like the stroke on the top of the graph is twice the width of the stroke on the bottom of the graph. Can we fix that so that are both just one pixel?
* In attachment 8784685 [details], the fill doesn't have an a border to indicate that the playhead never enters that region. I haven't followed the conversation, but I think that's still a good idea?
* We seem to be missing the lines that indicate iteration boundaries?
* What does the gradient in the top graph of attachment 8793578 [details] mean? Are these infinitely repeating animations? If so, can we show, e.g. 4 repeats and fade out the last two (so it's clear the animation keeps repeating?)
* I wonder if there's anything we can do to make the label look better when it overlaps the graph--it feels a bit messy at the moment.
* How does negative end-delay look? Did we do the dotted outline yet?

Comment 65

[Daisuke Akatsuka (:daisuke)]

(In reply to Brian Birtles (:birtles) from comment #64)
> Just a few drive-by comments:

Thanks, Brian!
I'll do that.

Just one thing.

> * What does the gradient in the top graph of attachment 8793578 [details]
> mean? Are these infinitely repeating animations? If so, can we show, e.g. 4
> repeats and fade out the last two (so it's clear the animation keeps
> repeating?)

As comment 45, I don't want to change the iteration count in TimeScale. So, please let me try to convertible iteration count at given displayable area from TimeScale.
I'll attach the screenshot after trying.

Thanks!

Comment 66

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Please let me cancel to review since I'll fix comment 64.
Thanks.

Comment 67

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6887dfe3529

Comment 68

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f248b3c432a

Comment 69

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

Review commit: https://reviewboard.mozilla.org/r/82706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82706/

Comment 70

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/4-5/

Comment 71

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/4-5/

Comment 72

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/4-5/

Comment 73

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/4-5/

Comment 74

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/4-5/

Comment 75

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/4-5/

Comment 76

[Daisuke Akatsuka (:daisuke)]

Attachment: 20161003-iterations.html.png

This is new screenshot.
Please confirm!

Thanks.

Comment 77

[Daisuke Akatsuka (:daisuke)]

Attachment: 20161003-negative-delay-enddelay.png

Also, this is a screenshot for negative delay and endDelay.

Comment 78

[Patrick Brosset <:pbro>]

Thanks for the new patches. I have started to review them.

I've also done some testing on the UI with all patches applied and found some bugs that I'll list here:

- If you have an animation that fills forward, and after it's been displayed in the tool you resize the sidebar to make it bigger, then the end of the graph is offset a little bit.
Say the animation lasts for 10 seconds, you have the linear-gradient displayed at the 10s mark, right? Now make the sidebar bigger (drag the splitter to the left), you should see that there's now a visible gap between the iteration and the linearGradient, and that they don't end/start at the 10s mark exactly.
If you then click on rewind for example, then that fixes everything because it forces a re-render.

- Let's say you have an animation to repeats twice (iterations:2) displayed in the tool. Now use the playbackRate drop down to change the rate from 1x to, say, 10x: you should see only 1 iteration anymore instead of 2. Same if you change it to 0.1x, you see the 2 iterations by they are not sized correctly. You can see the problem if you play the animation at the new rate.

- The logic to create the segments seems to need some fine-tuning for the end of a timing-function.
For instance, take a linear easing, it should look like a right-angled triangle where, when you reach a progress of 1, the line of the graph goes straight down back to 0 in a vertical line right?
Instead, it goes down in a steep slope, but a slope nevertheless, so it's not vertical. Not a big deal, I just thought it'd mention it anyway, because this may confuse users a little bit.

- The semi-transparent background behind animation names make the names more visible, but also ends up hiding part or all of the graph. If you have a long name, and a narrow sidebar, then there are big changes that the name is going to hide the graph.
This is not related to this patch though, I guess we've always had that problem, but now it's just more visible because we have a useful graph to look at, before we just had useless rectangles.
This wouldn't happen if we had a bigger place for the tool too (see bug 1246195) since it would be really rare for the timeline to be really small.
My take on this is that we should move it to the side, where the target DOM nodes are displayed, and maybe make the animations taller, to leave space for this, but this is unrelated to this bug, so don't worry about this.
Let's try and find a quick solution that doesn't make you loose time on this but also avoids the problem: maybe have a max-width on the name in % unit, so that as you resize the sidebar, you see just a cropped part of the title, which isn't a big deal because it's also in the tooltip.

- Related: if an animation has the light bolt icon (runs on the compositor thread), then the semi-transparent background that contains the title expands all the way to the right of the timeline. It shouldn't. This brackground should always only enclose the title, and the bolt icon should just sit there on the right-hand side on its own.

- Can you file a subsequent bug as per comment 44? So we also see the timing functions for css animations?

Comment 79

[Patrick Brosset <:pbro>]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

https://reviewboard.mozilla.org/r/74490/#review81710

We're getting really close on this part 1 patch. Just a few final comments. Mostly about code clarity.

::: devtools/client/animationinspector/components/animation-time-block.js:15
(Diff revision 5)
>  const {createNode, TimeScale} = require("devtools/client/animationinspector/utils");
>  
>  const { LocalizationHelper } = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/locale/animationinspector.properties");
>  
> +// The following constant variables are used in createPathSegments.

No need to say where these constants are used, their names should speak for themselves and it's easy enough for someone to search for them in the file to figure out where they are used.

::: devtools/client/animationinspector/components/animation-time-block.js:16
(Diff revision 5)
>  
>  const { LocalizationHelper } = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/locale/animationinspector.properties");
>  
> +// The following constant variables are used in createPathSegments.
> +// DURATION_RESOLUTION is the number that devides the duration to retrieve.

s/devides/divides

::: devtools/client/animationinspector/components/animation-time-block.js:16
(Diff revision 5)
> +// DURATION_RESOLUTION is the number that devides the duration to retrieve.
> +// This value shoud be integer and more than 2.
> +const DURATION_RESOLUTION = 4;

So, this constant is crucial to the algorithm that creates the segments for graph.
But 4 is sort of randomly choosen right?

A lower value would mean that we'd need to recurse more times in `createPathSegments`.

A higher value would mean that you'd recurse fewer times in `createPathSegments`.

so, really, 4 is choosen as a good compromise I guess.
I think the comment should explain this logic. Right now it's a bit hard to understand the logic in `createPathSegments` (see more comments about this below).

::: devtools/client/animationinspector/components/animation-time-block.js:19
(Diff revision 5)
> +// Threshold of whether subdivision is needed. The value shoud be 0 - 1.
> +const MIN_PROGRESS_THRESHOLD = 0.1;

Same comment here. The comment for this constant needs to explain more of the logic.
The idea is that while you create the graph, segment after segment, you check to see if there's a big gap between 2 consecutive segments on the Y axis (the progress). If the gap found is larger than this threshold, then you'd continue to recurse and create more segments.

So, 0.1 happens to work well here, because animations are 20px high, progress going from 0 to 1, it means that a threshold of 0.1 equals 2px, and a 2px change can't really be seen, so we're fine.

So, I think this comment should explain this. It took me a while to understand the overal logic, and I'd like to avoid other people to have to spend the same time.

::: devtools/client/animationinspector/components/animation-time-block.js:70
(Diff revision 5)
> +    // Create a dummy Animation timing data as the
> +    // state object we're being passed in.
> +    const timing = Object.assign({}, state, {
> +      iterations: state.iterationCount ? state.iterationCount : 1
> +    });

I think this should move to the `getSegmentHelper` function. It's a technical implementation detail that belongs to that function. Keeping it here unnecessarily complexifies the logic of the render method.

It looks like you use the `timing` object below though. Could you not use the `state` object below instead? I would really like to only see the `Object.assign` part inside `getSegmentHelper`.

::: devtools/client/animationinspector/components/animation-time-block.js:77
(Diff revision 5)
> +    const timing = Object.assign({}, state, {
> +      iterations: state.iterationCount ? state.iterationCount : 1
> +    });
> +
> +    // Get a helper function that returns the path segment of timing-function.
> +    const getSegment = getSegmentHelper(timing, this.win);

Rename `getSegment` to `segmentHelperFn` or something like this.

::: devtools/client/animationinspector/components/animation-time-block.js:85
(Diff revision 5)
> +    const minSegmentDuration = totalDuration / this.containerEl.clientWidth;
> +    // Minimum progress threshold.
> +    let minProgressThreshold = MIN_PROGRESS_THRESHOLD;
> +    // If the easing is step function,
> +    // minProgressThreshold should be changed by the steps.
> +    const stepFunction = timing.easing.match(/steps\((\d+)/);

Is `easing` garanteed to be `steps(N` or can it sometimes be `step-start` or `step-end`? In which case this regular expression wouldn't work.

::: devtools/client/animationinspector/components/animation-time-block.js:87
(Diff revision 5)
> +    let minProgressThreshold = MIN_PROGRESS_THRESHOLD;
> +    // If the easing is step function,
> +    // minProgressThreshold should be changed by the steps.
> +    const stepFunction = timing.easing.match(/steps\((\d+)/);
> +    if (stepFunction) {
> +      minProgressThreshold = 1 / (Number(stepFunction[1]) + 1);

I think we more commonly use `parseInt(stepFunction[1], 10)` in the codebase than casting to a Number.

::: devtools/client/animationinspector/components/animation-time-block.js:109
(Diff revision 5)
> +    // Append delay.
> +    if (timing.delay > 0) {
> +      const startSegment = getSegment(0);
> +      const endSegment = { x: timing.delay, y: startSegment.y };
> +      appendPathElement(summaryEl, [startSegment, endSegment], "delay-path");
> +    }

Can you break out all these sections into their own methods?
So this one could go in a `renderDelay` method below the main `render` method. And then the next section that appends the 1st section of iterations would go into a `renderFirstIteration`, and so on.

::: devtools/client/animationinspector/components/animation-time-block.js
(Diff revision 5)
>            "style": `left:${delayX}%;
>                      width:${delayW}%;`
>          }
>        });
>      }
> -

nit: why did you remove this extra new line. It helps space things out.

::: devtools/client/animationinspector/components/animation-time-block.js:363
(Diff revision 5)
> + * @param {Number} minProgressThreshold - Minimum progress threshold.
> + * @param {function} getSegment - The function of getSegmentHelper.
> + * @return {Array} path segments -
> + *                 [{x: {Number} time, y: {Number} progress}, ...]
> + */
> +function createPathSegments(startTime, endTime, minSegmentDuration,

As said in previous comments, the logic here is crucial, but also hard to understand at first. 
I think a few minor changes to comments, some formatting and some more newlines would greatly help.
I don't think anything substantial needs to be rewritten, the solution is quite clever in fact, so let's keep it this way, but make it easier for people to understand.

Let me suggest this instead:

```
function createPathSegments(startTime, endTime, minSegmentDuration, minProgressThreshold,
                            getSegment) {
  // If the duration is too short, early return.
  if (endTime - startTime < minSegmentDuration) {
    return [getSegment(startTime), getSegment(endTime)];
  }

  // Otherwise, start creating segments.
  let pathSegments = [];

  // Append the segment for the startTime position.
  const startTimeSegment = getSegment(startTime);
  pathSegments.push(startTimeSegment);
  let previousSegment = startTimeSegment;
  
  // Split the duration in equal intervals, and iterate over them.
  // See the definition of DURATION_RESOLUTION for more information about this.
  const interval = (endTime - startTime) / DURATION_RESOLUTION;
  for (let index = 1; index <= DURATION_RESOLUTION; index++) {
    // Create a segment for this interval.
    const currentSegment = getSegment(startTime + index * interval);
    
    // If the distance between the Y coordinate (the animation's progress) of the previous
    // segment and the Y coordinate of the current segment is too large, then recurse with
    // a smaller duration to get more details in the graph.
    if (Math.abs(currentSegment.y - previousSegment.y) > minProgressThreshold) {
      // Divide the current interval (excluding start and end bounds by adding/subtracting
      // 1ms).
      pathSegments = pathSegments.concat(
        createPathSegments(previousSegment.x + 1, currentSegment.x - 1,
                           minSegmentDuration, minProgressThreshold, getSegment));
    }

    pathSegments.push(currentSegment);
    previousSegment = currentSegment;
  }

  return pathSegments;
}
```

::: devtools/client/animationinspector/components/animation-time-block.js:443
(Diff revision 5)
> +  const newId = `fade-${ new Date().getTime() }`;
> +  // Get fill color from style.
> +  const fillColor = win.getComputedStyle(pathEl).fill;
> +  const gradientEl = createNode({
> +    parent: parentEl,
> +    namespace: "http://www.w3.org/2000/svg",

Move this as a constant at the top of the file and reuse it throughout the file.
```
const SVG_NS = "http://www.w3.org/2000/svg";
```

::: devtools/client/animationinspector/components/animation-time-block.js:454
(Diff revision 5)
> +      "stop-color": fillColor,
> +      "stop-opacity": 1,

`stop-color` and `stop-opacity` can be set in CSS too, so instead of adding attributes here you could move this to animationinspector.css like so:

```
.animation .fade stop {
  stop-color: var(--timeline-background-color);
  stop-opacity: 1;
}
.animation .fade stop:nth-child(2) {
  stop-opacity: 0;
}
```

(assuming that you would add the class `fade` to the `linearGradient` element, to make it more obvious what it is).

This way, the `--timeline-background-color` variable is reused, and you don't need the `getComputedStyle` to get the fill color, and the code of the `appendFadeEffect` gets smaller.

Comment 80

[Patrick Brosset <:pbro>]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

https://reviewboard.mozilla.org/r/74492/#review82166

I have a few minor comments and 2 proposals for some UI changes. See below.

::: devtools/client/animationinspector/components/animation-time-block.js:140
(Diff revision 5)
> +        iterationCount -= negativeDelayCount;
> +      }
>      }
>  
> -    // Append 1st section of iterations,
> -    // if this animation has decimal iterationStart.
> +    // Append 1st section of iterations.
> +    // This section is effected from decimal iterationStart.

Rephrase to: "This section is only useful in cases where iterationStart has decimals"

::: devtools/client/animationinspector/components/animation-time-block.js:157
(Diff revision 5)
>                             minProgressThreshold, getSegment);
>        appendPathElement(summaryEl, segments, "iteration-path");
>      }
>  
> -    // Append middle section of iterations.
> +    if (iterationCount === Infinity) {
> +      // If iterations is Infinity, we fill desplayed area by iterations path.

Rewording needed: "If the animation repeats infinitely, we fill the remaining area with iteration paths"

::: devtools/client/animationinspector/components/animation-time-block.js:159
(Diff revision 5)
> +      // Get the count to fill.
> +      // However the count is too large,
> +      // choose MAX_INFINITE_ANIMATIONS_ITERATIONS for the performance.

Some rewording here too: "Calculate the number of iterations to display, with a maximum of MAX_INFINITE_ANIMATIONS_ITERATIONS".

::: devtools/client/animationinspector/components/animation-time-block.js:177
(Diff revision 5)
> +                           minProgressThreshold, getSegment);
> +      appendPathElement(summaryEl, firstSegments, "iteration-path");
> +
> +      // Append other iterations. We can copy first segments
> +      const isAlternate = timing.direction.match(/alternate/);
> +      for (let iteration = 1; iteration < infinityIterationCount; iteration++) {

There's already a bunch of variables and properties around with names similar to `iteration` so I think it's just easier if you keep the name of the index as just `i`, which is what we often use in for loops anyway.

::: devtools/client/animationinspector/components/animation-time-block.js:196
(Diff revision 5)
> +      // Append fade effect to last half iterations.
> +      const count = Math.floor(infinityIterationCount / 2) + 1;
> +      const selector = `.iteration-path:nth-last-child(-n+${ count })`;
> +      const iterationPathEls = summaryEl.querySelectorAll(selector);
> +      const opacityPerIteration = 1 / iterationPathEls.length;
> +      iterationPathEls.forEach((iterationPathEl, index) => {
> +        const startOpacity = 1 - opacityPerIteration * index;
> +        const endOpacity = startOpacity - opacityPerIteration;
> +        appendFadeEffect(summaryEl, iterationPathEl,
> +                         startOpacity, endOpacity, this.win);
> +      });

I'd like to propose a simplification here:

show the first iteration as normal, and then all the other ones as `opacity:.3` (or a value close to this).

This way it's easier to differentiate infinite animations from animations that repeat a finite number of times. And visually, I'm not sure the gradient works so well after all.

I think that this change + the other one I mentioned about using solid color for forward filling animations would make the UI a bit cleaner.

::: devtools/client/animationinspector/components/animation-time-block.js:265
(Diff revision 5)
> -        (timing.endDelay > 0 ? timing.endDelay : 0) + minSegmentDuration;
> +               (timing.endDelay > 0 ? timing.endDelay : 0) + minSegmentDuration;
> -      const startSegment = getSegment(startTime);
> +        const startSegment = getSegment(startTime);
> -      const endSegment = { x: totalDuration, y: startSegment.y };
> +        const endSegment = { x: totalDisplayedDuration, y: startSegment.y };
> -      const pathEl = appendPathElement(summaryEl, [startSegment, endSegment],
> +        const pathEl = appendPathElement(summaryEl, [startSegment, endSegment],
> -                                       "fill-forwards-path");
> +                                         "fill-forwards-path");
> -      appendFadeEffect(summaryEl, pathEl, false, this.win);
> +        appendFadeEffect(summaryEl, pathEl, 1, 0, this.win);

I'm wondering if it really makes sense to use the fade effect for forward filling.
I think it might make sense to just have a solid color all the way to the end of the timeline instead of a gradient, and just keep the gradient for infinite animations instead. This way they would look less alike and easier for people to spot the difference.
In fact, a forward filling animation stays at its last progress position forever, so a solid color makes sense to me.

So for linear easing with forward filling, instead of having this:

  /|¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|
 / |                          |   
/  |                          |
¯¯¯ ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯

where the triangle would be green, and the rectangle would be a gradient,
we would have this:

  /¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|
 /                            |   
/                             |
¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
everything would be the same color green.

::: devtools/client/animationinspector/components/animation-time-block.js:530
(Diff revision 5)
>   * @return {Element} linearGradient element.
>   */
> -function appendFadeEffect(parentEl, pathEl, isStrokeNeeded, win) {
> +function appendFadeEffect(parentEl, pathEl,
> +                          startOpacity, endOpacity, win) {
>    // Create new id for linearGradient element.
>    const newId = `fade-${ new Date().getTime() }`;

I should have made that comment in part1, but just realized it here.
There are times where the ID is going to be the same for several linearGradient elements, so `new Date().getTime()` is not a guaranty to get unique IDs. You'd probably be better off just using a simple counter here.

It's a more visible problem in this patch though because it means that infinitely repeating animations may have several of their iterations using the same gradient, and therefore the effect is kind of broken, because you don't always see just one smooth gradient.

Comment 81

[Patrick Brosset <:pbro>]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

https://reviewboard.mozilla.org/r/77336/#review82170

Just a couple of things to change: moving some code and reusing common code.

::: devtools/client/animationinspector/components/animation-time-block.js:269
(Diff revision 5)
>                                           "fill-forwards-path");
>          appendFadeEffect(summaryEl, pathEl, 1, 0, this.win);
>        }
>      }
>  
> +    // Append auxiliary line for hidden progress.

Just like I said in part 1, these various sections need to go in their own methods. In this case, it would be `renderHiddenProgress` or something like this.

Also, this comment isn't the most self-explanatory I think. Maybe something like: "Append negative delays (which overlap the animation)" ?

::: devtools/client/animationinspector/components/animation-time-block.js:276
(Diff revision 5)
> +      const auxiliaryTimingDummayAnimation =
> +        new this.win.Animation(
> +          new this.win.KeyframeEffect(null, null, auxiliaryTiming), null);
> +      const auxiliaryTimingGetSegment = time => {
> +        auxiliaryTimingDummayAnimation.currentTime = time * timing.playbackRate;
> +        const progress =
> +          auxiliaryTimingDummayAnimation.effect.getComputedTiming().progress;
> +        return { x: time, y: Math.max(progress, 0) };
> +      };

This code is almost exactly like the one in `getSegmentHelper`. Please adapt that function so you can also reuse it here instead of duplicating the code.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_endDelay.js:53
(Diff revision 5)
> +
> +function checkPath(animationEl, state) {
> +  // Check existance of enddelay path.
> +  const endDelayPathEl = animationEl.querySelector(".enddelay-path");
> +  ok(endDelayPathEl, "The endDelay path should exist");
> +  // Check enddelay path coord.

nit: Please change `coord` with `coordinates`, it doesn't hurt using full words :)
Also, insert a newline before this comment, this separate things out better (same comment in the other test you touched).

::: devtools/client/themes/animationinspector.css:49
(Diff revision 5)
> +  --enddelay-hidden-progress-color: gray;
> +  /* The color of none fill mode */
> +  --fill-none-color: lightGray;

Can you check devtools' existing css variables here:
devtools\client\themes\variables.css and if you find a couple that match the style you're trying to achieve here, then use them instead of hard coding colors.
We're trying to keep the number of colors low so that we can eventually have a simple theming system. And we need most (all) of our colors to be in this variables file.

::: devtools/client/themes/animationinspector.css:430
(Diff revision 5)
> +.animation-timeline .animation .delay:after,
> +.animation-timeline .animation .end-delay:after {

nit: please use `::after` instead of `:after`

Comment 82

[Patrick Brosset <:pbro>]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

https://reviewboard.mozilla.org/r/77338/#review82174

The code changes in this patch look fine to me, so I'd like to R+ on this account, but see comment 78 in the bug. I think we need another way to show titles that doesn't impact the graph so much.

Comment 83

[Patrick Brosset <:pbro>]

Comment on attachment 8797057 [details]
Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

https://reviewboard.mozilla.org/r/82706/#review82176

::: devtools/client/animationinspector/animation-controller.js:288
(Diff revision 1)
> -    if (this.traits.hasSetCurrentTimes) {
> +    // Pausing and setting the current time on each player, one
> -      yield this.animationsFront.setCurrentTimes(this.animationPlayers, time,
> -                                                 shouldPause);
> -    } else {

I'm not sure I understand why you removed this first condition. The part in the `else` was only a fallback for backward compatibility in case the toolbox was connected to ano older devtools server that didn't have the `setCurrentTimes` method.

Removing this `if` is a huge drop in performance when you move the scrubber and when there are many animations, so I think you should make the changes you described in the comment further below both in the `setCurrentTimes` method in devtools/server/actors/animations.js and in the `else` part.

Comment 84

[Daisuke Akatsuka (:daisuke)]

Patrich, thank you very much!
I'll check and fix them.

Comment 85

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

https://reviewboard.mozilla.org/r/74490/#review81710

> Is `easing` garanteed to be `steps(N` or can it sometimes be `step-start` or `step-end`? In which case this regular expression wouldn't work.

We changed the serialization in bug 1264865 .

step-start -> steps(1, start)
step-end -> steps(1)

Comment 86

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=670592ada7eb

Comment 87

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/5-6/

Comment 88

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/5-6/

Comment 89

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/5-6/

Comment 90

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/5-6/

Comment 91

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/5-6/

Comment 92

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/5-6/

Comment 93

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8797057 [details]
Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82706/diff/1-2/

Comment 94

[Daisuke Akatsuka (:daisuke)]

I filed a bug for CSS Animation's timing-function in bug 1309468.

Comment 95

[Patrick Brosset <:pbro>]

Comment on attachment 8797057 [details]
Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

https://reviewboard.mozilla.org/r/82706/#review84076

Thanks, this looks good!

::: devtools/client/animationinspector/test/browser_animation_timeline_setCurrentTime.js:9
(Diff revision 2)
> +
> +"use strict";
> +
> +requestLongerTimeout(2);
> +
> +// Check setCurrentTime behavior.

Please be a little more explicit with this comment. This test doesn't check all possible cases for using setCurrenttime, but only a few very specific ones, right? So I think the comment should explain what they are.

Comment 96

[Patrick Brosset <:pbro>]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

https://reviewboard.mozilla.org/r/74490/#review84088

Nice. Thanks.
I haven't done as much manual testing as previous times. I'll first review all the other commits and then do some more tests which could be address in another commit later if needed.

::: devtools/client/animationinspector/components/animation-time-block.js:15
(Diff revision 6)
> +// createPathSegments method divides the given starting and ending time
> +// with DURATION_RESOLUTION number to get the transition of animation progress.
> +// But depending on timing-function, we may be not able to make the graph
> +// smoothly in case of the resolution.
> +// So, the method compares those progress of divided time.
> +// If the difference of the progress is more than MIN_PROGRESS_THRESHOLD,
> +// then it tries to re-divide.
> +// DURATION_RESOLUTION shoud be integer and more than 2.

Slight rephrasing:

```
// In the createPathSegments function, an animation duration is divided by
// DURATION_RESOLUTION in order to draw the way the animation progresses.
// But depending on the timing-function, we may be not able to make the graph smoothly
// progress if this resolution is not high enough.
// So, if the difference of animation progress between 2 divisions is more than
// MIN_PROGRESS_THRESHOLD, then createPathSegments re-divides by DURATION_RESOLUTION.
// DURATION_RESOLUTION shoud be integer and more than 2.
```

It feels clearer to me, but I'll let you decide what you want to use.

::: devtools/client/animationinspector/components/animation-time-block.js:540
(Diff revision 6)
> +    let svgEl;
> +    for (svgEl = parentEl; svgEl; svgEl = svgEl.parentNode) {
> +      if (svgEl.classList.contains("summary")) {
> +        break;
> +      }
> +    }

I think you want this simpler form instead:

`let svgEl = parentEl.closest(".summary");`

Comment 97

[Patrick Brosset <:pbro>]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

https://reviewboard.mozilla.org/r/74492/#review84098

One last pass needed I think. I requested 2 changes below. See my comments inline.

::: devtools/client/animationinspector/components/animation-time-block.js:84
(Diff revision 6)
>      let backgroundIterations = TimeScale.getIterationsBackgroundData(animation);
>  
> -    // Displayed total duration
> -    const totalDuration = TimeScale.getDuration() * state.playbackRate;
> +    // Animation summary graph element.
> +    const summaryEl = createNode({
> +      parent: this.containerEl,
> +      namespace: "http://www.w3.org/2000/svg",

Now that you have a `SVG_NS` const, you should us it here.

::: devtools/client/animationinspector/components/animation-time-block.js:94
(Diff revision 6)
> +    const totalDisplayedDuration =
> +      state.playbackRate * TimeScale.getDuration()
> +      / this.containerEl.clientWidth * summaryEl.getBoundingClientRect().width;

I think we should simplify this to be:
`const totalDisplayedDuration = state.playbackRate * TimeScale.getDuration();`

This way, the bug that I mentioned about the graph having a visible offset when resizing the panel will go away.
This also requires to have `width: 100%;` in the `.animation-timeline .animation .summary` rule.

So, I like this because:
- it fixes a bug
- it avoids having to deal with pixels in this file, where everything else is in % or ms
- it makes the code simpler.

It does introduce a sort of display bug where infinite animations and forwards filling animations don't extend all the way up to the right side of the panel.
But I'm willing to have this minor bug in favor of cleaner/simpler code.
We can think of ways to fix this display bug later.

::: devtools/client/animationinspector/components/animation-time-block.js:464
(Diff revision 6)
> +                       minProgressThreshold, getSegment);
> +  appendPathElement(parentEl, firstSegments, "iteration-path");
> +
> +  // Append other iterations. We can copy first segments
> +  // Also, append fade effect to last half iterations.
> +  // Fade element group.

I'm repeating something I said earlier in that I don't think we should be using a gradient for infinite animations. 
I've tested how it would look with simply a lower opacity for other iterations and I quite like it:
https://dl.dropboxusercontent.com/u/714210/faded-infinite-iterations.png

What I don't like about the gradient is that it's more visual noise, for little added value. And depending on how many iterations are displayed in the timeline, it may be weird to only start the gradient after the middle of the summary graph.

If we go ahead with the opacity solution instead, then it means you can remove the `fadeIterationCount` variable, the `gEl` element, and even the whole `appendFadeEffect` function (and related constants and CSS.
So a big code simplification too!

Open to discussion on this. But I do believe this makes the UI simpler.

::: devtools/client/themes/animationinspector.css:351
(Diff revision 6)
>  
>  /* Animation summary graph */
>  .animation-timeline .animation .summary {
>    position: absolute;
> -  width: 100%;
> +  /* To fit to right side of tool to show fill-mode or Infinity */
> +  width: calc(100% + var(--keyframes-marker-size));

As commented earlier, this should become `width:100%` even if that means we don't extend all the way to the right of the viewport, at least we always draw the graph correctly, even if you resize the panel.

Comment 98

[Patrick Brosset <:pbro>]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

https://reviewboard.mozilla.org/r/77336/#review84116

::: devtools/client/animationinspector/components/animation-time-block.js:376
(Diff revision 6)
>  
>  /**
>   * Render delay section.
>   * @param {Element} parentEl - Parent element of this appended path element.
>   * @param {Object} state - State of animation.
> - * @param {function} getSegment - The function of getSegmentHelper.
> + * @param {Object} segmentHelper - The object of getSegmentHelper.

Maybe rephrase to "The object returned by getSegmentHelper" (same below).

::: devtools/client/animationinspector/components/animation-time-block.js:597
(Diff revision 6)
> +
> +/**
>   * Get a helper function which returns the segment coord from given time.
>   * @param {Object} state - animation state
>   * @param {Object} win - window object
> - * @return {function} getSegmentHelper
> + * @param {Object} segmentHelper - The object of getSegmentHelper.

This should not be a @param but a @return:

`@return {Object}`

::: devtools/client/animationinspector/components/animation-time-block.js:613
(Diff revision 6)
> -  const endTime = dummyAnimation.effect.getComputedTiming().endTime;
> +
> +  // Returns segment helper object.
> +  return {
> +    animation: dummyAnimation,
> +    endTime: dummyAnimation.effect.getComputedTiming().endTime,
> +    // To fit the original animation behavior.

I'm not sure what that means.
I think comments about the properties that the segmentHelper object contains should be in the jsdoc above the function:

```
@return {Object} A segmentHelper object that has the following properties:
- animation: The script animation used to get the progress
- endTime: the end time of the animation
- timeFilter: ...
- ...
```

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_delay.js:79
(Diff revision 6)
> +  if (state.delay === 0) {
> +    ok(!delayPathEl, "The delay path for zero delay should not exist");
> +    return;
> +  }
> +  ok(delayPathEl, "The delay path should exist");
> +  // Check delay path coord.

nit: Please change `coord` with `coordinates`, it doesn't hurt using full words :)
Also, insert a newline before this comment, this separate things out better

Comment 99

[Patrick Brosset <:pbro>]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

https://reviewboard.mozilla.org/r/77338/#review84130

Ok to R+ this one, and work on a better solution for animation titles in bug 1309845

Comment 100

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c64fc6d7a1a

Comment 101

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/6-7/

Comment 102

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/6-7/

Comment 103

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/6-7/

Comment 104

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/6-7/

Comment 105

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/6-7/

Comment 106

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/6-7/

Comment 107

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8797057 [details]
Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82706/diff/2-3/

Comment 108

[Patrick Brosset <:pbro>]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

https://reviewboard.mozilla.org/r/74492/#review85496

Comment 109

[Patrick Brosset <:pbro>]

Attachment: animation-resets-on-play.gif

I found a bug while doing some more testing with the patches.
STR:
- open https://dl.dropboxusercontent.com/u/714210/iterations.html
- in the animation panel, press rewind
- then press play, and wait until the scrubber has gone past the 3s mark
- press pause now, and immediately press play again

==> As you can see, the animation that has 1 iteration only starts again. It should not, it should remain in its current position because it fills forward. Pressing pause and then play should not reset its time.

The same happens every time an animation ends as this GIF shows.

Because all the patches here have quite a few revisions already and are complex, I would prefer if this was fixed as part of a separate (part 8) patch, but I do think it needs to be fixed before landing this.

Comment 110

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #109)
> Created attachment 8802071 [details]
> animation-resets-on-play.gif
> 
> I found a bug while doing some more testing with the patches.

Thanks, Patrich!
I'll check and fix in this bug.

Comment 111

[Daisuke Akatsuka (:daisuke)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #110)
> (In reply to Patrick Brosset <:pbro> from comment #109)
> > Created attachment 8802071 [details]
> > animation-resets-on-play.gif
> > 
> > I found a bug while doing some more testing with the patches.
> 
> Thanks, Patrich!
> I'll check and fix in this bug.

This bug reproduce in Firefox 49.
I'll check a point that calculates the timing.
Maybe here??
https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/utils.js#137

Comment 112

[Patrick Brosset <:pbro>]

(In reply to Daisuke Akatsuka (:daisuke) from comment #111)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #110)
> > (In reply to Patrick Brosset <:pbro> from comment #109)
> > > Created attachment 8802071 [details]
> > > animation-resets-on-play.gif
> > > 
> > > I found a bug while doing some more testing with the patches.
> > 
> > Thanks, Patrich!
> > I'll check and fix in this bug.
> 
> This bug reproduce in Firefox 49.
> I'll check a point that calculates the timing.
> Maybe here??
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/
> animationinspector/utils.js#137
Oh you're right, sorry I missed this. Don't spend time on it then. Maybe file a bug for it if you have time, but no need to fix this in your bug. Let's land your changes!

Comment 113

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #112)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #111)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #110)
> > > (In reply to Patrick Brosset <:pbro> from comment #109)
> > > > Created attachment 8802071 [details]
> > > > animation-resets-on-play.gif
> > > > 
> > > > I found a bug while doing some more testing with the patches.
> > > 
> > > Thanks, Patrich!
> > > I'll check and fix in this bug.
> > 
> > This bug reproduce in Firefox 49.
> > I'll check a point that calculates the timing.
> > Maybe here??
> > https://dxr.mozilla.org/mozilla-central/source/devtools/client/
> > animationinspector/utils.js#137
> Oh you're right, sorry I missed this. Don't spend time on it then. Maybe
> file a bug for it if you have time, but no need to fix this in your bug.
> Let's land your changes!

Yeah!!

Also, I filed that bug in bug 1311280.
Thanks!

Comment 114

[Ryan VanderMeulen [:RyanVM]]

FYI, this hit a weird autoland error. I filed bug 1311727 for it since it was something gps hadn't seen before.

Comment 115

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95a6c178d88
Part 1: Display animations' timing-functions in the animation-inspector. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/589975f9a0db
Part 2: Changes Infinity expression. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/2227adce7ea6
Part 3: Changes delay and endDelay expression. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3e1ebb42c1
Part 4: Changes composite sign color and animation name. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc0b092cbd8
Part 5: Updates popup information. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/9441ce3d4832
Part 6: Removes extra codes. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d63848ac47e2
Part 7: Change setCurrentTimeAll behavior. r=pbro

Comment 116

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38159860&repo=mozilla-inbound

Comment 117

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7744ef67db6
Backed out changeset d63848ac47e2 
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb80516af2b
Backed out changeset 9441ce3d4832 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d428882047f8
Backed out changeset ccc0b092cbd8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa1bb56e8c5
Backed out changeset 3a3e1ebb42c1 
https://hg.mozilla.org/integration/mozilla-inbound/rev/08de379c0f98
Backed out changeset 2227adce7ea6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a256decb1dfa
Backed out changeset 589975f9a0db 
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a39b0cc21c
Backed out changeset e95a6c178d88 for frequent failures in own test (browser_animation_timeline_shows_iterations.js)

Comment 118

[Daisuke Akatsuka (:daisuke)]

(In reply to Carsten Book [:Tomcat] from comment #116)
> backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38159860&repo=mozilla-
> inbound

Okay, thanks.

Comment 119

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5e09dfd33c

Comment 120

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b337837d6860

Comment 121

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27173dbeeb0c

Comment 122

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789103 [details]
Bug 1210795 - Part 1: Display animations' timing-functions in the animation-inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74490/diff/7-8/

Comment 123

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74492/diff/7-8/

Comment 124

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789105 [details]
Bug 1210795 - Part 3: Changes delay and endDelay expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77336/diff/7-8/

Comment 125

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789106 [details]
Bug 1210795 - Part 4: Changes composite sign color and animation name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77338/diff/7-8/

Comment 126

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789107 [details]
Bug 1210795 - Part 5: Updates popup information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77340/diff/7-8/

Comment 127

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8789108 [details]
Bug 1210795 - Part 6: Removes extra codes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77342/diff/7-8/

Comment 128

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8797057 [details]
Bug 1210795 - Part 7: Change setCurrentTimeAll behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82706/diff/3-4/

Comment 129

[Daisuke Akatsuka (:daisuke)]

Hi Patric,
I modified the patch 2 for backing out (comment 116).
Could you review patch 2 again?

Thanks.

Comment 130

[Patrick Brosset <:pbro>]

Comment on attachment 8789104 [details]
Bug 1210795 - Part 2: Changes Infinity expression.

https://reviewboard.mozilla.org/r/74492/#review88078

Comment 131

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55ac24f3f8ff
Part 1: Display animations' timing-functions in the animation-inspector. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e3f96551347d
Part 2: Changes Infinity expression. r=pbro
https://hg.mozilla.org/integration/autoland/rev/4f2a78576e79
Part 3: Changes delay and endDelay expression. r=pbro
https://hg.mozilla.org/integration/autoland/rev/a1811cc95604
Part 4: Changes composite sign color and animation name. r=pbro
https://hg.mozilla.org/integration/autoland/rev/2cc8a31b03a8
Part 5: Updates popup information. r=pbro
https://hg.mozilla.org/integration/autoland/rev/2558ac9b4094
Part 6: Removes extra codes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/23f929543498
Part 7: Change setCurrentTimeAll behavior. r=pbro

Comment 132

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/55ac24f3f8ff
https://hg.mozilla.org/mozilla-central/rev/e3f96551347d
https://hg.mozilla.org/mozilla-central/rev/4f2a78576e79
https://hg.mozilla.org/mozilla-central/rev/a1811cc95604
https://hg.mozilla.org/mozilla-central/rev/2cc8a31b03a8
https://hg.mozilla.org/mozilla-central/rev/2558ac9b4094
https://hg.mozilla.org/mozilla-central/rev/23f929543498

Comment 133

[Patrick Brosset <:pbro>]

Clearing left over needinfo.

Comment 134

[Patrick Brosset <:pbro>]

FYI, I have created some demo/documentation material for this at https://github.com/mozdevs/devtools-demos/pull/6

Comment 135

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Work_with_animations with new videos and screenshots. Please let me know if this covers it.

(It looks very nice!)

Comment 136

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #135)
> I've updated
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Work_with_animations with new videos and screenshots. Please let me know if
> this covers it.
> 
> (It looks very nice!)
Thanks Will. Sorry you had to redo these videos and screenshots though. But they look great!

One note about this:
"From Firefox 52 onwards, the bar is shaped to reflect the timing function used for the animation. In the example above you can see that the first bar is concave, representing ease-in, and the second is convex, representing ease-out."

For now, the shape only maps to the `easing` property used in WebAnimations API animations. So, animations created from JS.
If you create a CSS animation and define the `animation-timing-function` to be `ease-in`, you'll still see a linear shape.
The reason for this is described in comment 22. I'll paste it here to make it easier:

> As you're probably aware, there are two levels of easing:
> effect-level easing and keyframe-level easing.
> As a result, it's possible for different properties to follow
> different timing functions.
> Somewhere along the way, we decided to make the summary view
> just show the effect-level easing/timing. For CSS animations,
> however, animation-timing-function maps to keyframe-level
> easing so it's not reflected in the summary view.
For info, there is some information about this at https://hacks.mozilla.org/2016/11/visualize-animations-easing-in-devtools/ in the "Effect easing vs. keyframe easing" section.

For animation-timing-function used in CSS animations, we'll need bug 1210796 to be fixed. So this will land later.
Maybe that part could be rephrased to:

"From Firefox 52 onwards, the bar is shaped to reflect the easing effect used for the animation. In the example above you can see that the first bar is concave, representing ease-in, and the second is convex, representing ease-out."

I don't think we need to go into too much details here. We can update that part again when bug 1210796 lands I guess.

Comment 137

[Will Bamberg [:wbamberg]]

Thanks Patrick! I've updated it with your suggestion.