Closed
Bug 1462229
Opened 6 years ago
Closed 6 years ago
Animation inspector becomes blank: "easing is undefined" in graph-helper.js
Categories
(DevTools :: Inspector: Animations, enhancement, P3)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jdescottes, Assigned: daisuke)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
During today's demo: STRs: - open about:newtab - open inspector > animation inspector - hovering very quickly on the tiles of about:newtab console.error: (new TypeError("easing is undefined", "resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/animation/utils/graph-helper.js", 242)) Probably called from: getPreferredProgressThreshold, we should guard it with: if (!keyframe.easing) { continue; } (same as in getPreferredProgressThresholdByKeyframes)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dakatsuka
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5325180a75df1424848bff573686f5ca5cf3cb29
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
I have updated the patch 1 since the last try had some problems in Windows. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3731faa879d839dbd8ea78b92d23476431d4a1a
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8980171 [details] Bug 1462229 - Part 1: Avoid updating the state of removed animation. https://reviewboard.mozilla.org/r/246328/#review253124 ::: devtools/client/inspector/animation/animation.js:308 (Diff revision 2) > animations.splice(index, 1); > animation.off("changed", this.onAnimationStateChanged); > } > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. s/proceeded. Because/proceeded since ::: devtools/client/inspector/animation/animation.js:310 (Diff revision 2) > } > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. > + // Because the scrubber position is related the currentTime. > + // Also, should not update the state of removed animations since React components Also, don't update the state ... ::: devtools/client/inspector/animation/animation.js:311 (Diff revision 2) > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. > + // Because the scrubber position is related the currentTime. > + // Also, should not update the state of removed animations since React components > + // may refer same instance still. may refer to the same instance still
Attachment #8980171 -
Flags: review?(gl) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8980172 [details] Bug 1462229 - Part 2: Avoid updating SummaryGraphPath if the component have been destroyed or other node have been selected while calling async function. https://reviewboard.mozilla.org/r/246330/#review253126
Attachment #8980172 -
Flags: review?(gl) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8980173 [details] Bug 1462229 - Part 3: Guard that not touching null object during creating graph after animation inspector destroyed. https://reviewboard.mozilla.org/r/246332/#review253136 ::: devtools/client/inspector/animation/animation.js:536 (Diff revision 2) > * @return {Animation} > * https://drafts.csswg.org/web-animations/#the-animation-interface > */ > simulateAnimation(keyframes, effectTiming, isElementNeeded) { > + if (!this.win) { > + throw Error("Animation inspector already destroyed"); I think we should just return silently since throwing an error is unhelpful in this ase. // Don't simulate animation if the animation inspector is already destroyed. if (!this.win) { return; } ::: devtools/client/inspector/animation/animation.js:650 (Diff revision 2) > }); > } > > updateState(animations) { > + if (!this.inspector) { > + console.error("Animation inspector already destroyed"); Remove the console.error. Add a comment like above. ::: devtools/client/inspector/animation/components/graph/ComputedTimingPath.js:59 (Diff revision 2) > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/graph/EffectTimingPath.js:43 (Diff revision 2) > - const simulatedAnimation = simulateAnimation(null, effectTiming, false); > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(null, effectTiming, false); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/graph/NegativePath.js:59 (Diff revision 2) > - const simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:87 (Diff revision 2) > const effect = { > duration, > fill: "forwards", > }; > + > + // simulateAnimation throws error if animation inspector has destroyed already. Remove this. ::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:190 (Diff revision 2) > const endKeyframe = keyframes[i + 1]; > + > + try { > - segments.push(...this.getPathSegments(startKeyframe, endKeyframe)); > + segments.push(...this.getPathSegments(startKeyframe, endKeyframe)); > + } catch (e) { > + console.error(e); Remove this.
Attachment #8980173 -
Flags: review?(gl) → review+
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8980174 [details] Bug 1462229 - Part 4: Add test for fast removal. https://reviewboard.mozilla.org/r/246334/#review253138 ::: devtools/client/inspector/animation/test/browser_animation_logic_mutations_fast.js:6 (Diff revision 2) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test whether the animation inspector will not crush when remove/add animations faster. s/crush/crash ::: devtools/client/inspector/animation/test/browser_animation_logic_mutations_fast.js:15 (Diff revision 2) > + const { inspector } = await openAnimationInspector(); > + > + info("Check state of the animation inspector after fast mutations"); > + await startFastMutations(tab); > + ok(inspector.panelWin.document.getElementById("animation-container"), > + "Animation inspector should live"); s/should/should be
Attachment #8980174 -
Flags: review?(gl) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
Thank you for the reviewing, Gabriel! I have updated the patches. Will land if the try was green. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb8a5c425b8580218c7ded7b36c368ed0766b80
Comment 20•6 years ago
|
||
Pushed by dakatsuka@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/453e9dd459a2 Part 1: Avoid updating the state of removed animation. r=gl https://hg.mozilla.org/integration/autoland/rev/452e577be2f4 Part 2: Avoid updating SummaryGraphPath if the component have been destroyed or other node have been selected while calling async function. r=gl https://hg.mozilla.org/integration/autoland/rev/6fa5b5963b06 Part 3: Guard that not touching null object during creating graph after animation inspector destroyed. r=gl https://hg.mozilla.org/integration/autoland/rev/443ff1adf8ab Part 4: Add test for fast removal. r=gl
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/453e9dd459a2 https://hg.mozilla.org/mozilla-central/rev/452e577be2f4 https://hg.mozilla.org/mozilla-central/rev/6fa5b5963b06 https://hg.mozilla.org/mozilla-central/rev/443ff1adf8ab
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•