1219611 - When animations end in the timeline, make sure the time-label shows the right time

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

When an animation ends, the time label stops as well. But the final time is different for each playback.

http://i.imgur.com/yBCaYAs.jpg
http://i.imgur.com/CCvBsc2.jpg
http://i.imgur.com/Xe2mOk2.jpg

The time label updates using a requestAnimationFrame loop on the client-side. At each iteration of the loop, we detect whether the animations are done or not. If they are, we stop the scrubber and time label, but this could be a few millis after the actual end. We should make sure we position the scrubber and time label exactly at the right end time.

This loop is in \devtools\client\animationinspector\components.js in startAnimatingScrubber()

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I'd be happy to work on this one. Can someone assign it to me please ? Thank you

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1219611.patch

Hi !
I think it's good like this.
I removed a test in displayTimelineCurrentTime, which prevented the time-label to show the animation end time ( because the animation was not paused nor infinite ). I think it was here to prevent the time-label to show a number higher than the animation length, but I'm not sure of this.

The tests all PASS, so I guess it's fine.

Let me know if something is wrong, thank you.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8713353 [details] [diff] [review]
Bug_1219611.patch

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

Looks good to me!

::: devtools/client/animationinspector/components/animation-timeline.js
@@ +356,5 @@
>      return this.animations.some(({state}) => !state.iterationCount);
>    },
>  
>    startAnimatingScrubber: function(time) {
>      let x = TimeScale.startTimeToDistance(time);

Maybe move this variable declaration just before the if where it's used, for better packing.

startAnimationScrubber: function(time) {
  let isOutOfBounds....
  let isAllPaused...
  let hasInfinite...

  let x = TimeSc....
  if (x > 100...
}

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1219611.patch

Edited to match Comment 3

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Pushed to TRY : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3288d4e4e1c4 

I don't know if I did it well though. 
On my bookmark for the bug ( where I have my commit for this patch ), I did : 

> hg qnew patch.try
> hg qref --message "try: -b do -p linux64,macosx64,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-o,mochitest-e10s-dt -t none"
> hg push --bookmark timelabel_1219611 -f try

I don't know if I should restrict the push to my bookmark only.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

The job is over. 
There is one test that failed ( only on  Linux x64 opt ) : 

> TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(255, 0, 153), expected rgb(255, 255, 85) 

But I don't think that it's because of my patch though.

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #5)
> Pushed to TRY :
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3288d4e4e1c4 
> 
> I don't know if I did it well though. 
> On my bookmark for the bug ( where I have my commit for this patch ), I did
> : 
> 
> > hg qnew patch.try
> > hg qref --message "try: -b do -p linux64,macosx64,win64,eslint-gecko -u xpcshell,mochitest-dt,mochitest-o,mochitest-e10s-dt -t none"
> > hg push --bookmark timelabel_1219611 -f try
> 
> I don't know if I should restrict the push to my bookmark only.
Looks like whatever you did worked! I don't think the --bookmark part was required anyway.
When you push to try, you *force* push (-f) with your current commit being the one with the try message. This will push all the commits that try hasn't seen before. So if you updated recently, you might be pushing commits that just landed on fx-team and that try hasn't seen before.
That's fine (there's one in your try push for instance: 8be8932a2e0f).

(In reply to Nicolas Chevobbe from comment #6)
> The job is over. 
> There is one test that failed ( only on  Linux x64 opt ) : 
> 
> > TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(255, 0, 153), expected rgb(255, 255, 85) 
> 
> But I don't think that it's because of my patch though.
Yup, seems unrelated, and there's already an intermittent failure bug opened to track this: bug 1244270.

Comment 8

[Patrick Brosset <:pbro>]

Landed on fxteam: https://hg.mozilla.org/integration/fx-team/rev/af13bfd0725a

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Great !
May I ask you if you are willing to vouch for me to be a Mozillian (  https://mozillians.org/en-US/u/chevobbe.nicolas/  ) or should I contribute more to consider it ?

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af13bfd0725a