1463372 - Keyframes progress bar should be based on createdTime

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, enhancement)

Description

[Daisuke Akatsuka (:daisuke)]

Although we fixed bug 1454392, the keyframes progress bar had not been based on createdTime yet. We need fix.

Comment 1

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

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

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1463372 - Part 2: Test for createdTime based keyframes progress bar.

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

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

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8979486 [details]
Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

https://reviewboard.mozilla.org/r/245654/#review252526

::: devtools/client/inspector/animation/components/KeyframesProgressBar.js
(Diff revision 1)
>  
>    updateOffset(currentTime, animation, timeScale) {
>      const {
> +      createdTime,
>        playbackRate,
> -      previousStartTime = 0,

Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8979487 [details]
Bug 1463372 - Part 2: Test for createdTime based keyframes progress bar.

https://reviewboard.mozilla.org/r/245656/#review252530

::: devtools/client/inspector/animation/test/browser_animation_keyframes-progress-bar_after-resuming.js:16
(Diff revision 1)
> +  const { animationInspector, panel } = await openAnimationInspector();
> +
> +  const scrubberPositions = [0, 0.25, 0.5, 0.75];
> +  const expectedPositions = [0, 0.25, 0.5, 0.75];
> +
> +  info("Check whether the keyframes progress bar position was correctly");

s/correctly/correct

::: devtools/client/inspector/animation/test/browser_animation_keyframes-progress-bar_after-resuming.js:19
(Diff revision 1)
> +  const expectedPositions = [0, 0.25, 0.5, 0.75];
> +
> +  info("Check whether the keyframes progress bar position was correctly");
> +  await assertPosition(panel, scrubberPositions, expectedPositions, animationInspector);
> +
> +  info("Check whether the keyframes progress bar position was correctly " +

s/correctly/correct

Comment 6

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8979486 [details]
Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

https://reviewboard.mozilla.org/r/245654/#review252526

> Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.

Thanks, Patrick.

Oh, I had done same way in bug 1454392 since actually we don't need to refer the variable, in case of latest version.
https://reviewboard.mozilla.org/r/242616/diff/4#index_header

How do you think?

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8979486 [details]
Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

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

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8979487 [details]
Bug 1463372 - Part 2: Test for createdTime based keyframes progress bar.

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

Comment 9

[Daisuke Akatsuka (:daisuke)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #6)
> Comment on attachment 8979486 [details]
> Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.
> 
> https://reviewboard.mozilla.org/r/245654/#review252526
> 
> > Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.
> 
> Thanks, Patrick.
> 
> Oh, I had done same way in bug 1454392 since actually we don't need to refer
> the variable, in case of latest version.
> https://reviewboard.mozilla.org/r/242616/diff/4#index_header
> 
> How do you think?

Let me make the patches to land as it is.
Then if we need to fix this change, I'd like to fix all at the same time.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1be8a677b22
Part 1: Introduce createdTime to keyframes progress bar. r=pbro
https://hg.mozilla.org/integration/autoland/rev/4d264b49431f
Part 2: Test for createdTime based keyframes progress bar. r=pbro

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b1be8a677b22
https://hg.mozilla.org/mozilla-central/rev/4d264b49431f

Comment 12

[Daniel Bodea [:danibodea]]

Hello,

Considering that the 3rd comment from bug 1464369, it seems that these 2 bugs are the same issue. 
Using the same steps to reproduce from there, this issue is now fixed Nightly v62.0a1 (20180528220216). Pausing and playing will not make the property scrubber behave incorrectly.
This issue has been validated on Windows 10, Ubuntu 16.04 LTS, and Mac OS X 10.12.6.