1155663 - Animation widgets in the animation panel should be synchronized along the time axis

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

This is the main change in the animation panel v3 (bug 1153271).

The goal is to change the current UI.
Right now, each displayed animation is its own widget that has its own timeline (moving at its own speed) and its own set of playback controls.
We instead want these widgets to share one set of playback controls (at the top), and be displayed as time blocks, synchronized, along a global timeline.

The first step is bug 1155661 which will give the top toolbar the playback controls we need.
Step 2 is adding a time graduation header and global timeline that moves at the right rate according to document.timeline.currentTime, and that is draggable so that all currently displayed animations can have their currentTime changed.
Step 3 is throwing most of the animation player widget's UI code to make time blocks instead.

Here's a draft mockup: https://dl.dropboxusercontent.com/u/714210/animation-inspector-v3-mockup.png

Comment 1

[Patrick Brosset <:pbro>]

Let's block bug 1155661 instead. I started implementing this one first.
In this bug I'll simply be changing the player widgets with animation timelines, I'll add the scrubber in a separate bug.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

Bug 1155663 - Show animations as synchronized time blocks in animation inspector

This is the first step towards the animation-inspector UI v3 (bug 1153271).
The new UI is still hidden behind a pref, and this change doesn't implement
everything that is in the current v2 UI.

This introduces a new Timeline graph to represent all currently animated nodes
below the currently selected node.
v2 used to show them as independent player widgets. With this patch, we now show
them as synchronized time blocks on a common time scale.

Each animation has a preview of the animated node in the left sidebar, and a time
block on the right, the width of which represents its duration. The animation name
is also displayed.

Repeating animations have all of their iterations represented.
Delayed animations have a preceding line that represents how much is the animation
delayed by.
Infinite animations are cut off on the right side.

There's also a time scale header and background that gives the user information
about how long do the animations last.

This change does *not* provide a way to know what's the currentTime nor a way to
set it yet.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

Bug 1155663 - Make current animationinspector tests pass with the timeline UI

Out of all existing animationinspector tests, those that still make sense with
the new timeline-based UI are now also run with the new UI pref on.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

Bug 1155663 - More tests for the timeline-based animation inspector UI

Comment 5

[Patrick Brosset <:pbro>]

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

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: time-overflow.png

A couple of quick notes after applying the patch:

1) Looks like the durations can end up overflowing their alloted space - http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
2) I can't seem to scroll the panel vertically when it overflows.  Easiest to reproduce with the split console opened, but happens even without it.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Created attachment 8615537 [details]
> time-overflow.png
> 
> A couple of quick notes after applying the patch:
> 
> 1) Looks like the durations can end up overflowing their alloted space -
> http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
> 2) I can't seem to scroll the panel vertically when it overflows.  Easiest
> to reproduce with the split console opened, but happens even without it.

Ah, now I see I hadn't flipped the devtools.inspector.animationInspectorV3 pref, so disregard that.  After flipping that pref, I do notice that the panel doesn't respond well to horizontal resizing - if I start off wide and shrink I can't see the content to the right anymore, and if I start off narrow then widen the timeline doesn't expand to use all of the available space.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > Created attachment 8615537 [details]
> > time-overflow.png
> > 
> > A couple of quick notes after applying the patch:
> > 
> > 1) Looks like the durations can end up overflowing their alloted space -
> > http://bgrins.github.io/devtools-demos/inspector/animation-timing.html
> > 2) I can't seem to scroll the panel vertically when it overflows.  Easiest
> > to reproduce with the split console opened, but happens even without it.
> 
> Ah, now I see I hadn't flipped the devtools.inspector.animationInspectorV3
> pref, so disregard that.

Actually, after unapplying the patch I can scroll again.  I believe the changes are breaking scrolling in the current v2 UI.

Comment 9

[Brian Grinstead [:bgrins]]

Another thing - open http://bgrins.github.io/devtools-demos/inspector/animation-timing.html and select the <body> element.  Then reload the page a couple of times.  It seems about every other page reload or so it shows the "No animations were found for the current element." message.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> After flipping that pref, I do notice that the
> panel doesn't respond well to horizontal resizing - if I start off wide and
> shrink I can't see the content to the right anymore, and if I start off
> narrow then widen the timeline doesn't expand to use all of the available
> space.
Correct, for now the graph doesn't re-render on resize. I have this on my TODO list but was considering tackling this in a follow-up bug, in the interest of landing something quickly and iterating.

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Actually, after unapplying the patch I can scroll again.  I believe the
> changes are breaking scrolling in the current v2 UI.
Alright, I hadn't seen that. I will get this fixed in this bug.

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Another thing - open
> http://bgrins.github.io/devtools-demos/inspector/animation-timing.html and
> select the <body> element.  Then reload the page a couple of times.  It
> seems about every other page reload or so it shows the "No animations were
> found for the current element." message.
I'll investigate this, I suspect a race condition between the panel init and the animation retrieval.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > After flipping that pref, I do notice that the
> > panel doesn't respond well to horizontal resizing - if I start off wide and
> > shrink I can't see the content to the right anymore, and if I start off
> > narrow then widen the timeline doesn't expand to use all of the available
> > space.
> Correct, for now the graph doesn't re-render on resize. I have this on my
> TODO list but was considering tackling this in a follow-up bug, in the
> interest of landing something quickly and iterating.
Filed bug 1171863 for this.
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > Another thing - open
> > http://bgrins.github.io/devtools-demos/inspector/animation-timing.html and
> > select the <body> element.  Then reload the page a couple of times.  It
> > seems about every other page reload or so it shows the "No animations were
> > found for the current element." message.
> I'll investigate this, I suspect a race condition between the panel init and
> the animation retrieval.
Ok, this happens with the current (v2) UI too, so I filed bug 1171861 to take care of this instead.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

https://reviewboard.mozilla.org/r/9819/#review9037

Cancelling review since this breaks scrolling in the current UI (comment 8).  I've also only lightly looked at the CSS changes because of this, I'll take a pass at those on the next round

::: browser/devtools/animationinspector/animation-panel.js:179
(Diff revision 1)
> -    // Otherwise, create player widgets.
> +    // Otherwise, create player widgets or render the AnimationsTimeline

This comment doesn't seem up to date -> doesn't look like the AnimationsTimeline is being rendered (or that anything is happening) when isNewUI

::: browser/devtools/animationinspector/components.js:808
(Diff revision 1)
> +        }).textContent = state.name;

I see lots of uses of this pattern in the code - it looks like it'd make sense to add textContent as an option to createNode to make this read simpler, and so that you could set the textContent and still assign the statement to a variable that contains the node.

::: browser/devtools/animationinspector/components.js:806
(Diff revision 1)
> +            "style": `left:${i * w}px`

If i === 0 here then you probably don't need the style attribute to be set (or you could just make it be "left: 0" if it's needed)

::: browser/devtools/animationinspector/components.js:770
(Diff revision 1)
> +      time.style.left = i + "px";

Why .style.left here instead of using the pattern used elsewhere?  Inside createNode attributes object, add "style": `left:${i}px;`

::: browser/devtools/animationinspector/components.js:760
(Diff revision 1)
> +    for (let i = 0; i < width; i += 4 * interval) {

What is this '4' for?  Unless if it's needed for some mathematical reason, may as well extract that into a named constant to avoid the need for a comment

::: browser/devtools/animationinspector/utils.js:114
(Diff revision 1)
> +      timingStep <<= 1;

Is this meant to be the same as `timingStep *= 2`?  If so, can you use that instead?

::: browser/devtools/animationinspector/utils.js:95
(Diff revision 1)
> + * Finds the optimal tick interval between time markers in this timeline.

This function needs the comment expanded.  What is an optimial tick interval?  What is dataScale used for - is that a width in px?

::: toolkit/devtools/server/actors/animation.js:85
(Diff revision 1)
> +    this.observer = new win.MutationObserver(this.onAnimationMutation);

I'm assuming we don't need to deal with subframe animations?  That would complicate things

::: browser/themes/shared/devtools/animationinspector.css:107
(Diff revision 1)
> +  background-image: -moz-element(#time-graduations);

This could use a comment about how document.mozSetImageElement is used to give this a dynamic background using canvas drawing since this isn't a commonly used thing

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> ::: toolkit/devtools/server/actors/animation.js:85
> (Diff revision 1)
> > +    this.observer = new win.MutationObserver(this.onAnimationMutation);
> 
> I'm assuming we don't need to deal with subframe animations?  That would
> complicate things
Yes, this was a deliberate choice I made when working on bug 1155651. You can see that the getAnimationPlayersForNode method (Animations actor) uses getAllAnimations without passing true as the 2nd argument, which restrict the search to nodes within the same frame.
The only thing that traverses frames is the global play/pause button at the top, because when you click it, you expect all animations on the page to pause/resume.
But when selecting a node in the tree, you want to see the animations below that node, and the most common use case will be that the animated nodes will be in the same frame, close to each other.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

Bug 1155663 - Show animations as synchronized time blocks in animation inspector

This is the first step towards the animation-inspector UI v3 (bug 1153271).
The new UI is still hidden behind a pref, and this change doesn't implement
everything that is in the current v2 UI.

This introduces a new Timeline graph to represent all currently animated nodes
below the currently selected node.
v2 used to show them as independent player widgets. With this patch, we now show
them as synchronized time blocks on a common time scale.

Each animation has a preview of the animated node in the left sidebar, and a time
block on the right, the width of which represents its duration. The animation name
is also displayed.

Repeating animations have all of their iterations represented.
Delayed animations have a preceding line that represents how much is the animation
delayed by.
Infinite animations are cut off on the right side.

There's also a time scale header and background that gives the user information
about how long do the animations last.

This change does *not* provide a way to know what's the currentTime nor a way to
set it yet.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8613909 [details]
MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

Bug 1155663 - Make current animationinspector tests pass with the timeline UI

Out of all existing animationinspector tests, those that still make sense with
the new timeline-based UI are now also run with the new UI pref on.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8613910 [details]
MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

Bug 1155663 - More tests for the timeline-based animation inspector UI

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

Bug 1155663 - Show animations as synchronized time blocks in animation inspector

This is the first step towards the animation-inspector UI v3 (bug 1153271).
The new UI is still hidden behind a pref, and this change doesn't implement
everything that is in the current v2 UI.

This introduces a new Timeline graph to represent all currently animated nodes
below the currently selected node.
v2 used to show them as independent player widgets. With this patch, we now show
them as synchronized time blocks on a common time scale.

Each animation has a preview of the animated node in the left sidebar, and a time
block on the right, the width of which represents its duration. The animation name
is also displayed.

Repeating animations have all of their iterations represented.
Delayed animations have a preceding line that represents how much is the animation
delayed by.
Infinite animations are cut off on the right side.

There's also a time scale header and background that gives the user information
about how long do the animations last.

This change does *not* provide a way to know what's the currentTime nor a way to
set it yet.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8613909 [details]
MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

Bug 1155663 - Make current animationinspector tests pass with the timeline UI

Out of all existing animationinspector tests, those that still make sense with
the new timeline-based UI are now also run with the new UI pref on.

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8613910 [details]
MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

Bug 1155663 - More tests for the timeline-based animation inspector UI

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: last-second-repeated.png

What I seen when inspecting the body on http://bgrins.github.io/devtools-demos/inspector/animation-timing.html.  Notice that 6s is repeated twice at the end

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8613909 [details]
MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

https://reviewboard.mozilla.org/r/9821/#review9453

The test changes look generally OK as long as it's green

::: browser/devtools/animationinspector/test/head.js:141
(Diff revision 2)
> +function assertAnimationsDisplayed(panel, nbAnimations, isNewUI=false, msg="") {

Could isNewUI be detected from the panel or pref value?  Then the function calls for old/new could be identical and removing the old code eventually may be a bit easier

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8613910 [details]
MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

https://reviewboard.mozilla.org/r/9823/#review9455

::: browser/devtools/animationinspector/test/browser.ini:36
(Diff revision 2)
> -[browser_animation_timeline_animates.js]
> +[browser_animation_timeline_displays_with_pref.js]

Were these files renamed (instead of added/deleted)?  I can't tell in the reviewboard UI

::: browser/devtools/animationinspector/test/browser_animation_timeline_ui.js:18
(Diff revision 2)
> +  ok(el.querySelectorAll(".time-header .time-tick").length,

I'd like to see a new test that covers the time graduations / tick interval stuff more thoroughly

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

https://reviewboard.mozilla.org/r/9819/#review9457

I see some issue with the time intervals in Comment 20.  I also have detected a performance issue with this when testing with a very large of iterations - try opening the following page:

data:text/html,<style>div { animation: slideshow 3s 10000; background: red;width:10px;height:10px; } @keyframes slideshow {0% {  } 100% { transform: translateX(500px); }}</style><div></div>

It will jank with the new inspector turned on (causing browser hangs if you bump up the iteration count even more).  Haven't profiled it yet, but I'm guessing it has to do with the node creation code.  Hopefully this situation won't come up in the wild since you'd probably just use infinite instead, but something to measure and make a decision about.

I understand this is preffed off by default so you may want to push some of these issues into follow ups - I think it's pretty close to being ready to land.  Just trying to be thorough to make sure any issues at least get tracked.

::: browser/themes/shared/devtools/animationinspector.css:152
(Diff revision 2)
> +  background-color: rgba(255,255,255,0.03);

I've been starting to store these types of things in variables for tool-specific needs:

.theme-dark {
  --even-animation-background-color: rgba(255,255,255,0.03);
  /* or some better name */
}

.theme-light {
  --even-animation-background-color: rgba(128,128,128,0.03);
}

.animation-timeline .animation:nth-child(2n) {
  background-color: var(--even-animation-background-color);
}

Comment 24

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8620507 [details]
> last-second-repeated.png
> 
> What I seen when inspecting the body on
> http://bgrins.github.io/devtools-demos/inspector/animation-timing.html. 
> Notice that 6s is repeated twice at the end
Good catch, it's because for long durations (seconds) I format the time with (time / 1000).toFixed(0) which of course means that there could be several times the same number displayed. I should add at least 1 fraction.

(In reply to Brian Grinstead [:bgrins] from comment #23)
> I also have
> detected a performance issue with this when testing with a very large of
> iterations - try opening the following page:
> 
> data:text/html,<style>div { animation: slideshow 3s 10000; background:
> red;width:10px;height:10px; } @keyframes slideshow {0% {  } 100% {
> transform: translateX(500px); }}</style><div></div>
> 
> It will jank with the new inspector turned on (causing browser hangs if you
> bump up the iteration count even more).  Haven't profiled it yet, but I'm
> guessing it has to do with the node creation code.  Hopefully this situation
> won't come up in the wild since you'd probably just use infinite instead,
> but something to measure and make a decision about.
You're right, I wonder if the right solution could be to just display one big iteration if there are more than, say, 50 or something. They're not going to be visible easily anyway, and it's not like users can really count the number of iterations just by looking at how many blocks are drawn. So maybe, the number should be displayed too.

> I understand this is preffed off by default so you may want to push some of
> these issues into follow ups - I think it's pretty close to being ready to
> land.  Just trying to be thorough to make sure any issues at least get
> tracked.
Thanks for being thorough Brian, it helps a lot. I'd rather spend a little bit more time fixing the important issues before landing than filing tones of follow-ups. The perf issue is definitely a blocker here. Other visual glitches can probably be done later though.

> ::: browser/themes/shared/devtools/animationinspector.css:152
> (Diff revision 2)
> > +  background-color: rgba(255,255,255,0.03);
> 
> I've been starting to store these types of things in variables for
> tool-specific needs:
> 
> .theme-dark {
>   --even-animation-background-color: rgba(255,255,255,0.03);
>   /* or some better name */
> }
> 
> .theme-light {
>   --even-animation-background-color: rgba(128,128,128,0.03);
> }
> 
> .animation-timeline .animation:nth-child(2n) {
>   background-color: var(--even-animation-background-color);
> }
Nice, thanks.

Comment 25

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #24)
> You're right, I wonder if the right solution could be to just display one
> big iteration if there are more than, say, 50 or something. They're not
> going to be visible easily anyway, and it's not like users can really count
> the number of iterations just by looking at how many blocks are drawn. So
> maybe, the number should be displayed too.
Or just get rid of the iterations elements, just have one iteration element, with a repeating linear gradient in it that creates the pattern to represent the iterations.

Comment 26

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> Comment on attachment 8613910 [details]
> MozReview Request: Bug 1155663 - More tests for the timeline-based animation
> inspector UI
> 
> https://reviewboard.mozilla.org/r/9823/#review9455
> 
> ::: browser/devtools/animationinspector/test/browser.ini:36
> (Diff revision 2)
> > -[browser_animation_timeline_animates.js]
> > +[browser_animation_timeline_displays_with_pref.js]
> 
> Were these files renamed (instead of added/deleted)?  I can't tell in the
> reviewboard UI
The word timeline was used before this patch in the tests, and is therefore now confusing. I'd like it to only refer to the way the new UI is displayed.
Here's the rename list in the patch:

rename from browser/devtools/animationinspector/test/browser_animation_timeline_waits_for_delay.js
rename to browser/devtools/animationinspector/test/browser_animation_playerWidgets_scrubber_delayed.js

rename from browser/devtools/animationinspector/test/browser_animation_timeline_is_enabled.js
rename to browser/devtools/animationinspector/test/browser_animation_playerWidgets_scrubber_enabled.js

rename from browser/devtools/animationinspector/test/browser_animation_timeline_animates.js
rename to browser/devtools/animationinspector/test/browser_animation_playerWidgets_scrubber_moves.js

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #25)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #24)
> > You're right, I wonder if the right solution could be to just display one
> > big iteration if there are more than, say, 50 or something. They're not
> > going to be visible easily anyway, and it's not like users can really count
> > the number of iterations just by looking at how many blocks are drawn. So
> > maybe, the number should be displayed too.
> Or just get rid of the iterations elements, just have one iteration element,
> with a repeating linear gradient in it that creates the pattern to represent
> the iterations.

I thought about this too.  It might be the easiest way to handle it if you can figure out how to do that, and assuming that we won't need mouse interaction with the individual iterations.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8613909 [details]
MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

https://reviewboard.mozilla.org/r/9821/#review9549

Ship It!

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

This is the first step towards the animation-inspector UI v3 (bug 1153271).
The new UI is still hidden behind a pref, and this change doesn't implement
everything that is in the current v2 UI.

This introduces a new Timeline graph to represent all currently animated nodes
below the currently selected node.
v2 used to show them as independent player widgets. With this patch, we now show
them as synchronized time blocks on a common time scale.

Each animation has a preview of the animated node in the left sidebar, and a time
block on the right, the width of which represents its duration. The animation name
is also displayed.

Repeating animations have all of their iterations represented.
Delayed animations have a preceding line that represents how much is the animation
delayed by.
Infinite animations are cut off on the right side.

There's also a time scale header and background that gives the user information
about how long do the animations last.

This change does *not* provide a way to know what's the currentTime nor a way to
set it yet.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8613909 [details]
MozReview Request: Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

Bug 1155663 - Make current animationinspector tests pass with the timeline UI; r=bgrins

Out of all existing animationinspector tests, those that still make sense with
the new timeline-based UI are now also run with the new UI pref on.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8613910 [details]
MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

Comment 32

[Patrick Brosset <:pbro>]

So, mozreview doesn't make things easy to follow on bugzilla at the moment, but I just attached new versions for the 3 patches. They should address all review comments.
I carried over the R+ on patch 2.
The other 2 are up for review.

Comment 33

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/9817/#review9595

::: browser/devtools/animationinspector/test/unit/test_findOptimalTimeInterval.js:45
(Diff revision 3)
> +  desc: "If 1ms corresponds to a distance that is greater than the min " +

'the min spacing'

::: browser/devtools/animationinspector/test/unit/test_findOptimalTimeInterval.js:51
(Diff revision 3)
> +  desc: "If 1ms corresponds to a distance that is greater than the min " +

'the min spacing'

::: browser/devtools/animationinspector/test/unit/test_findOptimalTimeInterval.js:58
(Diff revision 3)
> +  timeScale: 0.001,

I think this one could use a description - why does this end up as 10.24?

::: browser/devtools/animationinspector/test/unit/test_timeScale.js:125
(Diff revision 3)
> +  do_check_eq(TimeScale.minStartTime, Infinity);

Same comment as in the last test about switching from the old assertion style to the new ones

::: browser/devtools/animationinspector/test/browser_animation_timeline_header.js:13
(Diff revision 3)
> +const TIME_GRADUATION_MIN_SPACING = 40;

You could export and import this from components.js to prevent needing this comment.  Or is this intentionally duplicated?

::: browser/devtools/animationinspector/test/unit/test_findOptimalTimeInterval.js:80
(Diff revision 3)
> +      do_check_true(eval(expectedInterval));

Here is says the do_chec_* functions are deprectated and should be replaced with the normal ok / equal calls (which can take in a message so you would't need the if (desc) { do_print (desc); } anymore:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8613908 [details]
MozReview Request: Bug 1155663 - Show animations as synchronized time blocks in animation inspector; r=bgrins

https://reviewboard.mozilla.org/r/9819/#review9599

Ship It!

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8613910 [details]
MozReview Request: Bug 1155663 - More tests for the timeline-based animation inspector UI; r=bgrins

https://reviewboard.mozilla.org/r/9823/#review9601

Ship It!

Comment 36

[Patrick Brosset <:pbro>]

Attachment: bug1155663-1-synchronized-time-blocks.patch

Thanks Brian for the reviews.
I've addressed the last comments and folded part 1 and 2 together to avoid test failures when bisecting.

Comment 37

[Patrick Brosset <:pbro>]

Attachment: bug1155663-2-tests.patch

And part 2, also carrying R+ over.

Comment 38

[Patrick Brosset <:pbro>]

And a new try push with the latest patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15067d4e94c

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e8d7e7aa8e6a
https://hg.mozilla.org/integration/fx-team/rev/a8db6242385e

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8d7e7aa8e6a
https://hg.mozilla.org/mozilla-central/rev/a8db6242385e