1438755 - Avoid updating while the state of component is updating asynchronously

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Daisuke Akatsuka (:daisuke)]

The render function was called twice with different state ( one is previous state, another one is new state ) at componentWillReceiveProps calling. The reason is, there are places where updates the state asynchronously in componentWillReceiveProps, original thread goes forward updating the component while the state is updating. As result, sometime summary graph and keyframes graph could not make correctly due to order of selecting a node.
In this bug, address this problem.

Comment 1

[Daisuke Akatsuka (:daisuke)]

Also, the cause of that the graphs could not draw correctly may be that timeScale object did not update correctly. I'll investigate why the object did not update.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Hmm, it seems, the keyframes graph problem is happening on my local environment only.
Drawing is working well on Nightly and on today's m-c on other machines. (Maybe build environment?)

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

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

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.

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

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1438755 - Part 3: Use timeScale object in latest props.

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

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

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

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952068 [details]
Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

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

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952069 [details]
Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.

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

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952070 [details]
Bug 1438755 - Part 3: Use timeScale object in latest props.

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

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

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

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952068 [details]
Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

https://reviewboard.mozilla.org/r/221298/#review227570

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:39
(Diff revision 2)
>  
>      this.state = {
>        // Duration which can display in one pixel.
>        durationPerPixel: 0,
> +      // To avoid rendering while the state is updating
> +      // since we call async function in updateState.

s/call async/call an async

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:47
(Diff revision 2)
>        keyframesList: [],
>      };
>    }
>  
>    componentDidMount() {
> +    // No need to set updating flag since paint sequence finish at here.

No need to set isStateUpdating state since paint sequence is finish here.

::: devtools/client/inspector/animation/components/graph/SummaryGraphPath.js:167
(Diff revision 2)
>  
>      const thisEl = ReactDOM.findDOMNode(this);
>      const totalDuration = this.getTotalDuration(animation, timeScale);
>      const durationPerPixel = totalDuration / thisEl.parentNode.clientWidth;
>  
> -    this.setState({ durationPerPixel, keyframesList });
> +    this.setState({ durationPerPixel, isStateUpdating: false, keyframesList });

Can you put each props on new lines

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952068 [details]
Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

https://reviewboard.mozilla.org/r/221298/#review227576

::: commit-message-335bc:1
(Diff revision 2)
> +Bug 1438755 - Part 1: For SummaryGraphPath. r?gl

Change the commit message to:

Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously. r=gl

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952068 [details]
Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

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

Comment 14

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952069 [details]
Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.

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

Comment 15

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952070 [details]
Bug 1438755 - Part 3: Use timeScale object in latest props.

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

Comment 16

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

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

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

https://reviewboard.mozilla.org/r/221304/#review232660

::: commit-message-43e18:1
(Diff revision 3)
> +Bug 1438755 - Part 4: Add tests. r?gl

This needs a better commit message

Add unit test for ...

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_computed-timing-path_different-timescale.js:12
(Diff revision 3)
> +
> +add_task(async function () {
> +  await addTab(URL_ROOT + "doc_simple_animation.html");
> +  const { inspector, panel } = await openAnimationInspector();
> +
> +  info("Checking the path by different time scale");

Checking the path for different time scale

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_computed-timing-path_different-timescale.js:15
(Diff revision 3)
> +  const { inspector, panel } = await openAnimationInspector();
> +
> +  info("Checking the path by different time scale");
> +  await selectNodeAndWaitForAnimations(".no-compositor", inspector);
> +  const pathStringA = panel.querySelector(".animation-iteration-path").getAttribute("d");
> +  // Select animation which has different time scale from no-compositor.

This should be an info().

Add a new line before this.

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_computed-timing-path_different-timescale.js:17
(Diff revision 3)
> +  info("Checking the path by different time scale");
> +  await selectNodeAndWaitForAnimations(".no-compositor", inspector);
> +  const pathStringA = panel.querySelector(".animation-iteration-path").getAttribute("d");
> +  // Select animation which has different time scale from no-compositor.
> +  await selectNodeAndWaitForAnimations(".endDelayed", inspector);
> +  // Select no-compositor again.

This should be an info(). Also add a new line before this

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

https://reviewboard.mozilla.org/r/221304/#review232664

::: devtools/client/inspector/animation/test/browser_animation_summary-graph_computed-timing-path_different-timescale.js:6
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test for ComputedTimingPath by different time scale.

Test the Computed Timing Path component for different time scales.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952069 [details]
Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.

https://reviewboard.mozilla.org/r/221300/#review232666

::: devtools/client/inspector/animation/components/AnimatedPropertyList.js:28
(Diff revision 3)
>  
>    constructor(props) {
>      super(props);
>  
>      this.state = {
> -      animatedProperties: null
> +      animatedProperties: null,

Add a comment for this. What kind of object should this animatedProperties state represent?

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952070 [details]
Bug 1438755 - Part 3: Use timeScale object in latest props.

https://reviewboard.mozilla.org/r/221302/#review232668

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

https://reviewboard.mozilla.org/r/221304/#review232670

Comment 22

[Daisuke Akatsuka (:daisuke)]

I'd like to make this to land before other bugs (bug 1437730, bug 1438435 and bug 1438072).
Because want to fix bug 1445291 first. I guess, the patches in this bug will fix that.

To do that, we should make the patches to rebase to current m-c. Then try, then make them to land.

Thanks.

Comment 23

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952068 [details]
Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.

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

Comment 24

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952069 [details]
Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.

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

Comment 25

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952070 [details]
Bug 1438755 - Part 3: Use timeScale object in latest props.

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

Comment 26

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8952071 [details]
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.

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

Comment 27

[Daisuke Akatsuka (:daisuke)]

I'll push to the inbound after checking the try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9531d0c16b757369a84a2846c859f8f5a2948a

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c7a6edd317
Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/745933a8028e
Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/731c586c2399
Part 3: Use timeScale object in latest props. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0819e523cb
Part 4: Add unit test for different time scales of ComputedTimingPath component. r=gl

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/97c7a6edd317
https://hg.mozilla.org/mozilla-central/rev/745933a8028e
https://hg.mozilla.org/mozilla-central/rev/731c586c2399
https://hg.mozilla.org/mozilla-central/rev/4f0819e523cb