Closed Bug 1438755 Opened 6 years ago Closed 6 years ago

Avoid updating while the state of component is updating asynchronously

Categories

(DevTools :: Inspector: Animations, defect)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1438755 - Part 1: Avoid updating while the state of SummaryGraphPath component is updating asynchronously.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1438755 - Part 2: Avoid updating while the state of AnimatedPropertyList component is updating asynchronously.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1438755 - Part 3: Use timeScale object in latest props.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1438755 - Part 4: Add unit test for different time scales of ComputedTimingPath component.
59 bytes, text/x-review-board-request
gl
: review+
Details 
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.
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.
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 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
Attachment #8952068 - Flags: review?(gl) → review+
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 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 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 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?
Attachment #8952069 - Flags: review?(gl) → review+
Comment on attachment 8952070 [details]
Bug 1438755 - Part 3: Use timeScale object in latest props.

https://reviewboard.mozilla.org/r/221302/#review232668
Attachment #8952070 - Flags: review?(gl) → review+
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
Attachment #8952071 - Flags: review?(gl) → review+
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.
Pushed by dakatsuka@mozilla.com:
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: