1426194 - Animation Inspector freezes the browser

Status: RESOLVED FIXED

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

Description

[Ian Moody [:Kwan] (UTC+0)]

STR:
1) Go to https://duckduckgo.com/?q=css+animations
2) Open DevTools Inspector
3) Select the Animations panel
4) The browser is now frozen (page animations continue to visibly run though)

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bb63705ec95e878172e8f4c4628e11814683ffdc&tochange=c3530ca8ede501e49fdd9efb7e5d7f72df7f522b

Regressed by bug 1422218.

Comment 1

[Daisuke Akatsuka (:daisuke)]

Thank you for the reporting.
I could reproduce it.
Will fix as soon as possible, thanks!

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1426194 - Part 2: Add test.

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

Comment 4

[Daisuke Akatsuka (:daisuke)]

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

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8938273 [details]
Bug 1426194 - Part 2: Add test.

https://reviewboard.mozilla.org/r/209042/#review214762

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review214764

::: devtools/client/animationinspector/graph-helper.js:269
(Diff revision 1)
> +      // The computed value on the time which represents keyframe offset changes to the
> +      // value which is written in the keyframe.
> +      // However, we can set keyframes that have same offset.
> +      // In this case, keyframes A, B which is same offset animate as  the animation from
> +      // previous keyframe of A to A and from B to next B.
> +      // However, the both computed values will be value of B since the offsets are same.
> +      // To correspond this, each distance of keyframes calculate from time of start
> +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.

Sorry Daisuke, the code changes look good to me, but I don't understand this comment clearly enough to R+ this.
Could you try to explain to me again what's happening in the bug, and how do we solve it here. 
In fact, this explanation would be good to have as a multi-line commit message too.
I just want to be sure I R+ the right thing.
Thanks!

Comment 7

[Daisuke Akatsuka (:daisuke)]

(In reply to Patrick Brosset <:pbro> from comment #6)
> Comment on attachment 8938272 [details]
> Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.
> 
> https://reviewboard.mozilla.org/r/209040/#review214764
> 
> ::: devtools/client/animationinspector/graph-helper.js:269
> (Diff revision 1)
> > +      // The computed value on the time which represents keyframe offset changes to the
> > +      // value which is written in the keyframe.
> > +      // However, we can set keyframes that have same offset.
> > +      // In this case, keyframes A, B which is same offset animate as  the animation from
> > +      // previous keyframe of A to A and from B to next B.
> > +      // However, the both computed values will be value of B since the offsets are same.
> > +      // To correspond this, each distance of keyframes calculate from time of start
> > +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.
> 
> Sorry Daisuke, the code changes look good to me, but I don't understand this
> comment clearly enough to R+ this.
> Could you try to explain to me again what's happening in the bug, and how do
> we solve it here. 
> In fact, this explanation would be good to have as a multi-line commit
> message too.
> I just want to be sure I R+ the right thing.
> Thanks!

Thanks Patrick! Okay!

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

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

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8938273 [details]
Bug 1426194 - Part 2: Add test.

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

Comment 10

[Patrick Brosset <:pbro>]

59 merges to beta next week I believe, Daisuke, do you want me to review the new commits you pushed?

Comment 11

[Daisuke Akatsuka (:daisuke)]

Yes, thanks!

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review215760

Thank you for the long explanation :)

Comment 13

[Daisuke Akatsuka (:daisuke)]

Thank you very much!
I'll land soon.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4a89ebfbb40
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e4f2f93a78f0
Part 2: Add test. r=pbro

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/mozilla-central/rev/209dc7135305
Part 2: Add test. r=pbro

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
https://hg.mozilla.org/mozilla-central/rev/209dc7135305

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/e4a89ebfbb40
https://hg.mozilla.org/mozilla-central/rev/e4f2f93a78f0