1261561 - endDelay incorrectly positioned for animations with multiple iterations

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect, P1)

Description

[Patrick Brosset <:pbro>]

Attachment: Screen Shot 2016-04-02 at 8.17.34 AM.png

STR:
- create an animation that has more than 1 iteration
- apply an endDelay to it
- visualize the animation in the animation-inspector

The endDelay is placed at the end of the first iteration, instead of at the end of the animation.

See screenshot.

Comment 1

[Patrick Brosset <:pbro>]

I think this should be a very simple fix.
Here's the code where we calculate the start x position for the endDelay:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/utils.js#326
it doesn't seem to take the number of iterations. If instead of doing |x + w| we did  |x + iterationW| I think that would solve it.

Marking as a P1 because we need to get this fixed before bug 1253494 ships out, but since it's a simple fix, I'm not assigning it to me just now and marking myself as mentor instead.

Please feel free to pick this up, by first going through our contribution guide: https://developer.mozilla.org/en-US/docs/Tools/Contributing

If no one takes it before some time next week, I'll do.

Comment 2

[Ryo Kato [:r_kato]]

I hear about this from :hiro, so I'd like to work on!

Could you assign this bug to me...?

Comment 3

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1261561 - Part 1: Fix TimeScale.getAnimationDimensions() to handle multiple iterations r?pbro

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

Comment 4

[Ryo Kato [:r_kato]]

I tried writing tests for getAnimationDimensions() in test/unit/test_timeScale.js, but that didn't work well... Could you tell me where to write tests?

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Ryo Kato [:r_kato] from comment #4)
> I tried writing tests for getAnimationDimensions() in
> test/unit/test_timeScale.js, but that didn't work well... Could you tell me
> where to write tests?
Yes, this is the right approach for the test. However test_timeScale.js is already quite a big test. I would advise creating another xpcshell test in the same folder. Maybe test_timeScale_dimensions.js and use it to test the output of the getAnimationDimensions method specifically.
What exactly did you mean by "that didn't work well"? Could you give more details so I can help you better?

Also, are you planning to push a separate patch for the test, or amend the first one? 
If a separate patch, then I guess I can review the first one right away.

Comment 6

[Ryo Kato [:r_kato]]

(In reply to Patrick Brosset [:pbro] from comment #5)
> I would advise creating another xpcshell test in the same folder.
> Maybe test_timeScale_dimensions.js and use it to test the output
> of the getAnimationDimensions method specifically.

> Also, are you planning to push a separate patch for the test, or
> amend the first one? 

Thank you for advice! I will write a separate test as "Part 2:" patch,
so I won't amend "Part 1:" patch.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8737823 [details]
MozReview Request: Bug 1261561 - Part 1: Fix TimeScale.getAnimationDimensions() to handle multiple iterations r?pbro

https://reviewboard.mozilla.org/r/44135/#review40937

Thanks. This looks good.

Comment 8

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

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

Comment 9

[Ryo Kato [:r_kato]]

Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

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

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

https://reviewboard.mozilla.org/r/44339/#review41491

::: devtools/client/animationinspector/test/unit/test_timeScale_dimensions.js:37
(Diff revision 2)
> +  }],
> +  expectedEndDelayX: 90
> +}];
> +
> +function run_test() {
> +  do_print("Test calcurating endDelayX");

s/calcurating/calculating

Comment 11

[Ryo Kato [:r_kato]]

Comment on attachment 8738121 [details]
MozReview Request: Bug 1261561 - Part 2: Add a xpcshell test for TimeScale.getAnimationDimensions() r=pbro

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

Comment 12

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/44339/#review41491

> s/calcurating/calculating

Thank you for reviewing! I've just fixed that typo.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I did push patches here to try server on behalf of r_kato.  There were two tries since I did forget to run xpcshell tests first.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

The result seem good.  Adding checkin-needed.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6c1e3c4a7f49
https://hg.mozilla.org/integration/fx-team/rev/21a9c5fb6e0c

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c1e3c4a7f49
https://hg.mozilla.org/mozilla-central/rev/21a9c5fb6e0c