Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678713002:
Correctly handle cancelling compositor animations initiated from main thread. (Closed)
|
Created:
4 years, 10 months ago by ymalik Modified:
4 years, 10 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, kinuko+watch, rjwright, shans, skobes Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrectly handle cancelling compositor animations initiated from main thread.
We can have another scroll come in while there is an animation that is waiting
to be aborted on the compositor. This CL adds handling for this case.
This CL also ensures that we only update compositor animations from the
updateCompositorAnimations function, where the document lifecycle state is
CompositingClean.
BUG=584067
Committed: https://crrev.com/4b15b712a5d86526647475da30467cd3ae3c5cb3
Cr-Commit-Position: refs/heads/master@{#374313}
Patch Set 1 #
Total comments: 9
Patch Set 2 : review comments #
Messages
Total messages: 17 (6 generated)
ymalik@chromium.org changed reviewers: + ajuma@chromium.org
https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:111: targetPos.moveBy(pixelDelta); @ajuma This is speculatively fix up the assert failure on L128. Presumably what's happening is that it's in state WaitingToCancelOnCompositor. I suspect that a user scroll is cancelled and another user scroll happens before UpdateCompositorState can clear the animation. This is possible right? https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:198: { I don't think we should ever be in a state where we have m_compositorAnimationId, and are not running on the compositor. I copied this over from programmatic scroll animator at the time. In the case of a programmatic scroll, we could be in WaitingToSendToCompositor if another programmatic scroll happens while we're running on the compositor. In this case, we either update the target right from SA::UserScroll or go into state RunningOnCompositorButNeedsUpdate. WDYT?
https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html (right): https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html:22: shouldBecomeEqual("document.scrollingElement.scrollTop < 40 && " + Won't this condition be true the first time it's checked (regardless of which scroll ends up "winning")? https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:111: targetPos.moveBy(pixelDelta); On 2016/02/07 21:23:37, ymalik1 wrote: > @ajuma > > This is speculatively fix up the assert failure on L128. Presumably what's > happening is that it's in state WaitingToCancelOnCompositor. I suspect that a > user scroll is cancelled and another user scroll happens before > UpdateCompositorState can clear the animation. This is possible right? Assuming that userScroll can get called at arbitrary times, yes. https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:116: abortAnimation(); It looks like this could touch compositing state in a situation where the document lifecycle state isn't CompositingClean. There's probably an assert disabler somewhere that's preventing an ASSERT from triggering for this. (And there's another abortAnimation call above that's similarly problematic.) Anything involving updating things on the compositor should happen inside of ::updateCompositorAnimations. https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:198: { On 2016/02/07 21:23:37, ymalik1 wrote: > I don't think we should ever be in a state where we have > m_compositorAnimationId, and are not running on the compositor. I copied this > over from programmatic scroll animator at the time. In the case of a > programmatic scroll, we could be in WaitingToSendToCompositor if another > programmatic scroll happens while we're running on the compositor. In this case, > we either update the target right from SA::UserScroll or go into state > RunningOnCompositorButNeedsUpdate. > > WDYT? Sounds reasonable.
@Ali Addressed feedback. PTAL :) https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html (right): https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html:22: shouldBecomeEqual("document.scrollingElement.scrollTop < 40 && " + On 2016/02/08 15:05:34, ajuma wrote: > Won't this condition be true the first time it's checked (regardless of which > scroll ends up "winning")? Yes. I added some expectations to check that we're in the correct cancel state. Also moved to virtual/threaded and added exception to not run on mac. https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:116: abortAnimation(); On 2016/02/08 15:05:35, ajuma wrote: > It looks like this could touch compositing state in a situation where the > document lifecycle state isn't CompositingClean. There's probably an assert > disabler somewhere that's preventing an ASSERT from triggering for this. (And > there's another abortAnimation call above that's similarly problematic.) > Anything involving updating things on the compositor should happen inside of > ::updateCompositorAnimations. Ah right. That's why we have the whole state machine to begin with. Handling this and the above case from ::updateCompositorAnimations. https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:198: { On 2016/02/08 15:05:35, ajuma wrote: > On 2016/02/07 21:23:37, ymalik1 wrote: > > I don't think we should ever be in a state where we have > > m_compositorAnimationId, and are not running on the compositor. I copied this > > over from programmatic scroll animator at the time. In the case of a > > programmatic scroll, we could be in WaitingToSendToCompositor if another > > programmatic scroll happens while we're running on the compositor. In this > case, > > we either update the target right from SA::UserScroll or go into state > > RunningOnCompositorButNeedsUpdate. > > > > WDYT? > > Sounds reasonable. Based on your feedback above, I moved the call to abortAnimation into ::updateComposiorAnimations. Now we can be in a state when m_compositorAnimationId exists, and the state is WaitingToSendToCompositor (We're in WaitingToCancelOnCompositor and another user scroll arrives). Thus, this change should be undone.
On 2016/02/08 18:41:23, ymalik1 wrote: > @Ali > > Addressed feedback. PTAL :) > > https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html > (right): > > https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html:22: > shouldBecomeEqual("document.scrollingElement.scrollTop < 40 && " + > On 2016/02/08 15:05:34, ajuma wrote: > > Won't this condition be true the first time it's checked (regardless of which > > scroll ends up "winning")? > > Yes. I added some expectations to check that we're in the correct cancel state. > > Also moved to virtual/threaded and added exception to not run on mac. > > https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): > > https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:116: > abortAnimation(); > On 2016/02/08 15:05:35, ajuma wrote: > > It looks like this could touch compositing state in a situation where the > > document lifecycle state isn't CompositingClean. There's probably an assert > > disabler somewhere that's preventing an ASSERT from triggering for this. (And > > there's another abortAnimation call above that's similarly problematic.) > > Anything involving updating things on the compositor should happen inside of > > ::updateCompositorAnimations. > > Ah right. That's why we have the whole state machine to begin with. Handling > this and the above case from ::updateCompositorAnimations. > > https://codereview.chromium.org/1678713002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:198: { > On 2016/02/08 15:05:35, ajuma wrote: > > On 2016/02/07 21:23:37, ymalik1 wrote: > > > I don't think we should ever be in a state where we have > > > m_compositorAnimationId, and are not running on the compositor. I copied > this > > > over from programmatic scroll animator at the time. In the case of a > > > programmatic scroll, we could be in WaitingToSendToCompositor if another > > > programmatic scroll happens while we're running on the compositor. In this > > case, > > > we either update the target right from SA::UserScroll or go into state > > > RunningOnCompositorButNeedsUpdate. > > > > > > WDYT? > > > > Sounds reasonable. > > Based on your feedback above, I moved the call to abortAnimation into > ::updateComposiorAnimations. Now we can be in a state when > m_compositorAnimationId exists, and the state is WaitingToSendToCompositor > (We're in WaitingToCancelOnCompositor and another user scroll arrives). > > Thus, this change should be undone. Thanks, lgtm.
ymalik@chromium.org changed reviewers: + jbroman@chromium.org
+Jeremy for Source/platform
I trust Ali's judgment about the functionality change, but could you please change the description to be more...descriptive? It looks to me like there's a functionality change here, but the description suggests that you're just reformatting or removing dead code or something.
Description was changed from ========== Clean up blink Scroll Animator code. BUG=584067 ========== to ========== Correctly handle cancelling compositor animations initiated from main thread. We can have another scroll come in while there is an animation that is waiting to be aborted on the compositor. This CL adds handling for this case. This CL also ensures that we only update compositor animations from the updateCompositorAnimations function, where the document lifecycle state is CompositingClean. BUG=584067 ==========
On 2016/02/08 20:19:20, jbroman wrote: > I trust Ali's judgment about the functionality change, but could you please > change the description to be more...descriptive? > > It looks to me like there's a functionality change here, but the description > suggests that you're just reformatting or removing dead code or something. Totally. Done!
rs lgtm
The CQ bit was checked by ymalik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678713002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678713002/20001
Message was sent while issue was closed.
Description was changed from ========== Correctly handle cancelling compositor animations initiated from main thread. We can have another scroll come in while there is an animation that is waiting to be aborted on the compositor. This CL adds handling for this case. This CL also ensures that we only update compositor animations from the updateCompositorAnimations function, where the document lifecycle state is CompositingClean. BUG=584067 ========== to ========== Correctly handle cancelling compositor animations initiated from main thread. We can have another scroll come in while there is an animation that is waiting to be aborted on the compositor. This CL adds handling for this case. This CL also ensures that we only update compositor animations from the updateCompositorAnimations function, where the document lifecycle state is CompositingClean. BUG=584067 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Correctly handle cancelling compositor animations initiated from main thread. We can have another scroll come in while there is an animation that is waiting to be aborted on the compositor. This CL adds handling for this case. This CL also ensures that we only update compositor animations from the updateCompositorAnimations function, where the document lifecycle state is CompositingClean. BUG=584067 ========== to ========== Correctly handle cancelling compositor animations initiated from main thread. We can have another scroll come in while there is an animation that is waiting to be aborted on the compositor. This CL adds handling for this case. This CL also ensures that we only update compositor animations from the updateCompositorAnimations function, where the document lifecycle state is CompositingClean. BUG=584067 Committed: https://crrev.com/4b15b712a5d86526647475da30467cd3ae3c5cb3 Cr-Commit-Position: refs/heads/master@{#374313} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4b15b712a5d86526647475da30467cd3ae3c5cb3 Cr-Commit-Position: refs/heads/master@{#374313} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
