Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(575)

Issue 845543007: cc: Fix scroll offset after scroll animation is canceled (Closed)

Created:
5 years, 11 months ago by ajuma
Modified:
5 years, 10 months ago
CC:
cc-bugs_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: Fix scroll offset after scroll animation is canceled When a scroll offset animation is removed by the main thread and a new scroll position is set, this new position should propagate to the pending tree on commit, and then to the active tree on activation. However, the removed animation continues running on the compositor thread until activation, producing a scroll delta. Currently, this scroll delta gets added to the new scroll position (on both threads). This CL prevents the scroll delta produced by a removed animation from being added to the new post-animation scroll position. BUG=243871 Committed: https://crrev.com/e17c4ca518cf85f598d07e61bade581c52ccb492 Cr-Commit-Position: refs/heads/master@{#311895}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+222 lines, -16 lines) Patch
M cc/animation/layer_animation_controller.h View 1 chunk +2 lines, -0 lines 0 comments Download
M cc/animation/layer_animation_controller.cc View 3 chunks +21 lines, -11 lines 0 comments Download
M cc/animation/layer_animation_controller_unittest.cc View 1 1 chunk +45 lines, -0 lines 0 comments Download
M cc/animation/layer_animation_value_observer.h View 1 chunk +1 line, -0 lines 0 comments Download
M cc/layers/layer.h View 2 chunks +2 lines, -0 lines 0 comments Download
M cc/layers/layer.cc View 3 chunks +14 lines, -3 lines 0 comments Download
M cc/layers/layer_impl.h View 3 chunks +4 lines, -0 lines 0 comments Download
M cc/layers/layer_impl.cc View 5 chunks +15 lines, -1 line 0 comments Download
M cc/test/animation_test_common.h View 1 2 chunks +9 lines, -0 lines 0 comments Download
M cc/test/animation_test_common.cc View 2 chunks +7 lines, -1 line 0 comments Download
M cc/trees/layer_tree_host_unittest_animation.cc View 1 2 1 chunk +102 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (4 generated)
ajuma
5 years, 11 months ago (2015-01-15 21:24:55 UTC) #2
Ian Vollick
On 2015/01/15 21:24:55, ajuma wrote: lgtm!
5 years, 11 months ago (2015-01-16 14:52:10 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/845543007/40001
5 years, 11 months ago (2015-01-16 14:53:14 UTC) #5
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 11 months ago (2015-01-16 14:56:12 UTC) #6
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/e17c4ca518cf85f598d07e61bade581c52ccb492 Cr-Commit-Position: refs/heads/master@{#311895}
5 years, 11 months ago (2015-01-16 14:56:59 UTC) #7
aelias_OOO_until_Jul13
I noticed this change conflicts with the scrolling cleanup I'm trying to do in https://codereview.chromium.org/800613009 ...
5 years, 11 months ago (2015-01-20 03:19:08 UTC) #9
ajuma
On 2015/01/20 03:19:08, aelias wrote: > I noticed this change conflicts with the scrolling cleanup ...
5 years, 11 months ago (2015-01-20 19:05:47 UTC) #10
aelias_OOO_until_Jul13
OK, thanks for the explanation, I see why it's needed. I'd prefer we didn't introduce ...
5 years, 11 months ago (2015-01-20 20:27:14 UTC) #11
ajuma
On 2015/01/20 20:27:14, aelias wrote: > OK, thanks for the explanation, I see why it's ...
5 years, 11 months ago (2015-01-20 21:07:45 UTC) #12
ajuma
On 2015/01/20 21:07:45, ajuma wrote: > On 2015/01/20 20:27:14, aelias wrote: > > OK, thanks ...
5 years, 11 months ago (2015-01-21 16:39:57 UTC) #13
danakj
Hi, This seems to cause crashes because we scroll on the active&pending tree, then on ...
5 years, 10 months ago (2015-01-31 00:07:21 UTC) #15
danakj
On 2015/01/31 00:07:21, danakj wrote: > Hi, > > This seems to cause crashes [when ...
5 years, 10 months ago (2015-01-31 00:07:41 UTC) #16
aelias_OOO_until_Jul13
Hmm, maybe we could just cease the animation as soon as the commit happens, instead ...
5 years, 10 months ago (2015-01-31 00:50:39 UTC) #17
danakj
On Fri, Jan 30, 2015 at 4:50 PM, <aelias@chromium.org> wrote: > Hmm, maybe we could ...
5 years, 10 months ago (2015-01-31 00:51:43 UTC) #18
ajuma
On 2015/01/31 00:50:39, aelias wrote: > Hmm, maybe we could just cease the animation as ...
5 years, 10 months ago (2015-01-31 00:55:33 UTC) #19
danakj
On Fri, Jan 30, 2015 at 4:55 PM, <ajuma@chromium.org> wrote: > On 2015/01/31 00:50:39, aelias ...
5 years, 10 months ago (2015-01-31 00:57:35 UTC) #20
aelias_OOO_until_Jul13
On 2015/01/31 at 00:55:33, ajuma wrote: > On 2015/01/31 00:50:39, aelias wrote: > > Hmm, ...
5 years, 10 months ago (2015-01-31 01:01:13 UTC) #21
danakj
On Fri, Jan 30, 2015 at 5:01 PM, <aelias@chromium.org> wrote: > On 2015/01/31 at 00:55:33, ...
5 years, 10 months ago (2015-01-31 01:04:08 UTC) #22
aelias_OOO_until_Jul13
5 years, 10 months ago (2015-01-31 01:17:39 UTC) #23
Message was sent while issue was closed.
Hmm, anyway, after my SyncedProperty scroll refactoring
(https://codereview.chromium.org/800613009 ) lands (should be soon), the best
way to do the current approach would be to add an explicit "clobber" boolean to
SyncedProperty that makes PendingDelta() always return identity.  That would be
equivalent to Dana's original proposal to  make the pending tree's delta not be
updated by the animation.

Powered by Google App Engine
This is Rietveld 408576698