Remove touch scroll chaining from compositor scrolling.

Remove touch scroll chaining from compositor scrolling.

This leaves a bunch of potential cleanup, but we'll see whether or not we want to land this before performing the cleanup.

BUG=526462
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a3b162b8f887c5cfa5c68d5bb89a89769c92d7f2
Cr-Commit-Position: refs/heads/master@{#351346}

[tdresser, 2015-09-03 15:16:04]

tdresser@chromium.org changed reviewers:
+ aelias@chromium.org, jdduke@chromium.org

[tdresser, 2015-09-03 15:16:05]

Main thread patch here:
https://codereview.chromium.org/1320543006

This doesn't attempt to perform much of the related cleanup.
I did a small amount of digging through our bugs, and as far as I can tell, most
of the chaining related issues we've seen have been due to cases where we chain
when we shouldn't.

Jared, do you think this is reasonable?

[tdresser, 2015-09-03 15:17:56]

On 2015/09/03 15:16:05, tdresser wrote:
> Main thread patch here:
> https://codereview.chromium.org/1320543006
> 
> This doesn't attempt to perform much of the related cleanup.
> I did a small amount of digging through our bugs, and as far as I can tell,
most
> of the chaining related issues we've seen have been due to cases where we
chain
> when we shouldn't.
> 
> Jared, do you think this is reasonable?

My plan is to land this and listen for any outcry. Assuming it sticks, we'll
perform the related cleanup in a milestone or two.

[jdduke (slow), 2015-09-03 15:28:04]

On 2015/09/03 15:17:56, tdresser wrote:
> On 2015/09/03 15:16:05, tdresser wrote:
> > Main thread patch here:
> > https://codereview.chromium.org/1320543006
> > 
> > This doesn't attempt to perform much of the related cleanup.
> > I did a small amount of digging through our bugs, and as far as I can tell,
> most
> > of the chaining related issues we've seen have been due to cases where we
> chain
> > when we shouldn't.
> > 
> > Jared, do you think this is reasonable?
> 
> My plan is to land this and listen for any outcry. Assuming it sticks, we'll
> perform the related cleanup in a milestone or two.

crbug.com/328217 comes to mind when thinking about the impact this would have. I
remember a good number of charts/graphs on wikipedia that can scroll by small
amounts on the vertical axis. Without any kind of bubbling when you start a
fling, it makes scrolling the page very tedious. Of course, I failed to list any
actual urls on that bug, so now I need to see if I can find some example cases.

Would it be weird to still allow a fling to "re-latch" to the first scrolled
layer? I think that would address my primary complaint here.

[tdresser, 2015-09-03 15:32:03]

On 2015/09/03 15:28:04, jdduke wrote:
> On 2015/09/03 15:17:56, tdresser wrote:
> > On 2015/09/03 15:16:05, tdresser wrote:
> > > Main thread patch here:
> > > https://codereview.chromium.org/1320543006
> > > 
> > > This doesn't attempt to perform much of the related cleanup.
> > > I did a small amount of digging through our bugs, and as far as I can
tell,
> > most
> > > of the chaining related issues we've seen have been due to cases where we
> > chain
> > > when we shouldn't.
> > > 
> > > Jared, do you think this is reasonable?
> > 
> > My plan is to land this and listen for any outcry. Assuming it sticks, we'll
> > perform the related cleanup in a milestone or two.
> 
> crbug.com/328217 comes to mind when thinking about the impact this would have.
I
> remember a good number of charts/graphs on wikipedia that can scroll by small
> amounts on the vertical axis. Without any kind of bubbling when you start a
> fling, it makes scrolling the page very tedious. Of course, I failed to list
any
> actual urls on that bug, so now I need to see if I can find some example
cases.
> 
> Would it be weird to still allow a fling to "re-latch" to the first scrolled
> layer? I think that would address my primary complaint here.

I accidentally ended up in that state during development, and found it extremely
disconcerting. I can upload a patch with that behavior if you want to try it
out.

[jdduke (slow), 2015-09-03 15:42:30]

On 2015/09/03 15:32:03, tdresser wrote:
> On 2015/09/03 15:28:04, jdduke wrote:
> > Would it be weird to still allow a fling to "re-latch" to the first scrolled
> > layer? I think that would address my primary complaint here.
> 
> I accidentally ended up in that state during development, and found it
extremely
> disconcerting. I can upload a patch with that behavior if you want to try it
> out.

What do you mean you ended up in that state? You allowed flings to relatch, and
found that disconcerting? Or the lack of relatching?

[tdresser, 2015-09-03 15:48:18]

On 2015/09/03 15:42:30, jdduke wrote:
> On 2015/09/03 15:32:03, tdresser wrote:
> > On 2015/09/03 15:28:04, jdduke wrote:
> > > Would it be weird to still allow a fling to "re-latch" to the first
scrolled
> > > layer? I think that would address my primary complaint here.
> > 
> > I accidentally ended up in that state during development, and found it
> extremely
> > disconcerting. I can upload a patch with that behavior if you want to try it
> > out.
> 
> What do you mean you ended up in that state? You allowed flings to relatch,
and
> found that disconcerting? Or the lack of relatching?

I tested a patch where flings relatch, and it was super odd.
I much prefer the feel of not having flings relatch.

[jdduke (slow), 2015-09-03 16:26:19]

On 2015/09/03 15:48:18, tdresser wrote:
> On 2015/09/03 15:42:30, jdduke wrote:
> > On 2015/09/03 15:32:03, tdresser wrote:
> > > On 2015/09/03 15:28:04, jdduke wrote:
> > > > Would it be weird to still allow a fling to "re-latch" to the first
> scrolled
> > > > layer? I think that would address my primary complaint here.
> > > 
> > > I accidentally ended up in that state during development, and found it
> > extremely
> > > disconcerting. I can upload a patch with that behavior if you want to try
it
> > > out.
> > 
> > What do you mean you ended up in that state? You allowed flings to relatch,
> and
> > found that disconcerting? Or the lack of relatching?
> 
> I tested a patch where flings relatch, and it was super odd.
> I much prefer the feel of not having flings relatch.

Hmm, I guess if scrolls don't chain then relatching at fling start would
definitely feel weird. What sites were you testing? Did the proposed (no
chaining at all) patch feel OK?

[tdresser, 2015-09-03 16:33:41]

On 2015/09/03 16:26:19, jdduke wrote:
> On 2015/09/03 15:48:18, tdresser wrote:
> > On 2015/09/03 15:42:30, jdduke wrote:
> > > On 2015/09/03 15:32:03, tdresser wrote:
> > > > On 2015/09/03 15:28:04, jdduke wrote:
> > > > > Would it be weird to still allow a fling to "re-latch" to the first
> > scrolled
> > > > > layer? I think that would address my primary complaint here.
> > > > 
> > > > I accidentally ended up in that state during development, and found it
> > > extremely
> > > > disconcerting. I can upload a patch with that behavior if you want to
try
> it
> > > > out.
> > > 
> > > What do you mean you ended up in that state? You allowed flings to
relatch,
> > and
> > > found that disconcerting? Or the lack of relatching?
> > 
> > I tested a patch where flings relatch, and it was super odd.
> > I much prefer the feel of not having flings relatch.
> 
> Hmm, I guess if scrolls don't chain then relatching at fling start would
> definitely feel weird. What sites were you testing? Did the proposed (no
> chaining at all) patch feel OK?

Thus far I've only tested synthetic test pages. This behavior should make us
align with Safari's behavior, and Firefox's current behavior, so I suspect
there's nothing that feels terrible.

I think this will be most irritating when there are small scrollers which only
scroll a tiny bit. Previously this would have only slowed down your scroll of
the parent a bit, whereas you're now forced to start a new scroll gesture. This
could maybe happen with input fields? I'm not aware of any specific examples of
this happening though.

[jdduke (slow), 2015-09-03 16:51:58]

On 2015/09/03 16:33:41, tdresser wrote:
> Thus far I've only tested synthetic test pages. This behavior should make us
> align with Safari's behavior, and Firefox's current behavior, so I suspect
> there's nothing that feels terrible.
> 
> I think this will be most irritating when there are small scrollers which only
> scroll a tiny bit. Previously this would have only slowed down your scroll of
> the parent a bit, whereas you're now forced to start a new scroll gesture.
This
> could maybe happen with input fields? I'm not aware of any specific examples
of
> this happening though.

Right, that was the issue I noticed on wikipedia. I'll see if I can find some
real repro cases.

[Rick Byers, 2015-09-03 19:54:27]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-09-03 19:54:28]

Please make sure whatever you land here is consistent with main thread.  I.e. if
we're disabling chaining on main only for touchscreen scrolls at the momement
(https://codereview.chromium.org/1320543006/), then let's make the cc side
touchscreen specific for now too.

[tdresser, 2015-09-04 14:15:05]

On 2015/09/03 19:54:28, Rick Byers wrote:
> Please make sure whatever you land here is consistent with main thread.  I.e.
if
> we're disabling chaining on main only for touchscreen scrolls at the momement
> (https://codereview.chromium.org/1320543006/), then let's make the cc side
> touchscreen specific for now too.

This only impacts touchscreen. We currently don't maintain any state while
performing touchpad scrolling. Each wheel event receives a separate scrollbegin
and scrollend. We'll need to fix this to disable touchpad chaining.

See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...
for details.

[Rick Byers, 2015-09-08 14:59:50]

rbyers@chromium.org changed reviewers:
- aelias@chromium.org

[tdresser, 2015-09-09 13:48:03]

Ping!

I'll hold off on landing the main thread patch until this is also ready to land.

[Rick Byers, 2015-09-09 13:48:44]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-09-09 13:49:41]

BTW, I accidentally added myself as reviewer - moved to cc again.  Jared (and/or
aelias/vollick) is the right reviewer for cc side scrolling code.

[jdduke (slow), 2015-09-09 14:41:09]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, bokan@chromium.org

[jdduke (slow), 2015-09-09 14:41:10]

Question about wheel scrolling. Technically wheel scrolls should still bubble,
right? In practice the gesture itself bubbles because each update locks to its
own layer. Consequently, if we prevent a single update from bubbling, there will
often be a motion discontinuity at the scroll layer boundaries, creating
synthetic jank.

I'm not sure we have the platform data we need (yet? maybe we have it on
Mac/CroS?) to be able to make touchpad scroll sequences latch to a single
scrolling layer.

https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2440: // The inner viewport layer represents
the viewport during scrolling.
Should we instead move this adjustment into
FindScrollLayerForDeviceViewportPoint?

[tdresser, 2015-09-10 13:56:51]

There's some talk of using mouse wheel transactions to lock mousewheel to scroll
a single layer. We'd assume that a sequence of mouse wheel events in close
succession should target the same scrollable area without bubbling.

https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2440: // The inner viewport layer represents
the viewport during scrolling.
On 2015/09/09 14:41:10, jdduke wrote:
> Should we instead move this adjustment into
> FindScrollLayerForDeviceViewportPoint?

Absolutely, thanks.
Done.

[Rick Byers, 2015-09-10 14:04:29]

On 2015/09/10 13:56:51, tdresser wrote:
> There's some talk of using mouse wheel transactions to lock mousewheel to
scroll
> a single layer. We'd assume that a sequence of mouse wheel events in close
> succession should target the same scrollable area without bubbling.

Let's not worry to much about this yet.  I'm not convinced that's a good idea. 
Next step after touchscreen, I think, is to better support touchpad (where we do
have clear begin/end signals) and see how that goes.  Physical wheel can stay as
is for quite some time I think - until we've really learned from changing the
touchscreen and touchpad behavior.
 
>
https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1306193006/diff/1/cc/trees/layer_tree_host_im...
> cc/trees/layer_tree_host_impl.cc:2440: // The inner viewport layer represents
> the viewport during scrolling.
> On 2015/09/09 14:41:10, jdduke wrote:
> > Should we instead move this adjustment into
> > FindScrollLayerForDeviceViewportPoint?
> 
> Absolutely, thanks.
> Done.

[jdduke (slow), 2015-09-10 14:45:06]

https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2391: should_bubble_scrolls_ = (type !=
NON_BUBBLING_GESTURE);
So I think we'll want to make this:

should_bubble_scrolls_ = (type == WHEEL);

and continue supporting bubbling for wheel events?

[tdresser, 2015-09-10 16:27:08]

On 2015/09/10 14:45:06, jdduke wrote:
>
https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (left):
> 
>
https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2391: should_bubble_scrolls_ = (type !=
> NON_BUBBLING_GESTURE);
> So I think we'll want to make this:
> 
> should_bubble_scrolls_ = (type == WHEEL);
> 
> and continue supporting bubbling for wheel events?

The wheel vs touchpad distinction is super confusing here.

You're stating that we'll get a jank when a touchpad scroll bubbles, because
we'll ignore part of the scroll delta that switches between scrollables?

That's already our behavior, isn't it?
See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

[jdduke (slow), 2015-09-10 16:58:26]

On 2015/09/10 16:27:08, tdresser wrote:
> The wheel vs touchpad distinction is super confusing here.
> 
> You're stating that we'll get a jank when a touchpad scroll bubbles, because
> we'll ignore part of the scroll delta that switches between scrollables?
> 
> That's already our behavior, isn't it?
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Ugg... I still don't really understand why we have that code when we also have
the bubbling logic. Sigh, I  guess this wouldn't be much of a change in behavior
then except for diagonal touchpad scrolls, and even then the difference would be
quite subtle. This is fine, but I'm not an owner.

[tdresser, 2015-09-11 12:43:23]

aelias@, PTAL.

[aelias_OOO_until_Jul13, 2015-09-11 18:06:02]

lgtm

[tdresser, 2015-09-11 19:08:54]

bokan@, can you make sure that the logic relating to the virtual viewport is
sane?

[tdresser, 2015-09-15 15:02:01]

bokan@: Ping!

[bokan, 2015-09-15 15:29:30]

Sorry, missed the original message.

LGTM

https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1306193006/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1519: // to the other. Scrolls
shouldn't bubbling from the child.
Nit: bubbling -> bubble

[jdduke (slow), 2015-09-16 21:02:17]

also lgtm

[tdresser, 2015-09-25 14:06:53]

Dave, can you take another look to make sure this still makes sense with the
changes to viewport scroll ordering?

In particular, LayerTreeHostImplVirtualViewportTest.FlingScrollBubblesToInner
looks a bit odd. Should we ensure that the current scrolling layer can only be
one of the inner or outer viewports?

[bokan, 2015-09-25 16:49:59]

On 2015/09/25 14:06:53, tdresser wrote:
> Dave, can you take another look to make sure this still makes sense with the
> changes to viewport scroll ordering?
> 
> In particular, LayerTreeHostImplVirtualViewportTest.FlingScrollBubblesToInner
> looks a bit odd. Should we ensure that the current scrolling layer can only be
> one of the inner or outer viewports?

I *think* it generally shouldn't matter as long as the viewport is scrolled
through the cc::Viewport class and not on one of the cc::LayerImpls directly.
That said, I do think it would make it simpler to reason about if the current
scrolling layer could only be one or the other always. I took a quick look at
the uses of currently scrolling layer, here's some notes:

LayerTreeHostImpl::ScrollAnimated - I think this one will scroll the LayerImpls
so that's probably a problem already. Also, this one also bubbles using
Layer->parent() which has some subtle bugs. It should use
NextLayerInScrollOrder. See crbug.com/481372 for why. Anyway, it's not aware
that it should skip the outer and use cc::Viewport on inner. If you have energy
remaining, this would be nice to fix. Otherwise, I'll file a bug to do this
later.

LayerTreeHostImpl::ScrollVerticallyByPage - Same as above.

LayerTreeHostImpl::PinchGestureBegin - Will set to outer, pretty sure it's ok to
set to inner here.

LayerTreeHostImpl::IsCurrentlyScrollingRoot and
LayerTreeHostImpl::IsCurrentlyScrollingLayerAt - Treats the two interchangeably,
if you do make it so that it can only be Inner, we should ASSERT it's not the
outer here. Better yet, assert in SetCurrentlyScrollingLayer.

LayerTreeHostImpl::FlingScrollBegin() - Checks for either outer or inner.

[tdresser, 2015-09-25 17:32:13]

On 2015/09/25 16:49:59, bokan wrote:
> On 2015/09/25 14:06:53, tdresser wrote:
> > Dave, can you take another look to make sure this still makes sense with the
> > changes to viewport scroll ordering?
> > 
> > In particular,
LayerTreeHostImplVirtualViewportTest.FlingScrollBubblesToInner
> > looks a bit odd. Should we ensure that the current scrolling layer can only
be
> > one of the inner or outer viewports?
> 
> I *think* it generally shouldn't matter as long as the viewport is scrolled
> through the cc::Viewport class and not on one of the cc::LayerImpls directly.
> That said, I do think it would make it simpler to reason about if the current
> scrolling layer could only be one or the other always. I took a quick look at
> the uses of currently scrolling layer, here's some notes:
> 
> LayerTreeHostImpl::ScrollAnimated - I think this one will scroll the
LayerImpls
> so that's probably a problem already. Also, this one also bubbles using
> Layer->parent() which has some subtle bugs. It should use
> NextLayerInScrollOrder. See crbug.com/481372 for why. Anyway, it's not aware
> that it should skip the outer and use cc::Viewport on inner. If you have
energy
> remaining, this would be nice to fix. Otherwise, I'll file a bug to do this
> later.
> 
> LayerTreeHostImpl::ScrollVerticallyByPage - Same as above.
> 
> LayerTreeHostImpl::PinchGestureBegin - Will set to outer, pretty sure it's ok
to
> set to inner here.
> 
> LayerTreeHostImpl::IsCurrentlyScrollingRoot and
> LayerTreeHostImpl::IsCurrentlyScrollingLayerAt - Treats the two
interchangeably,
> if you do make it so that it can only be Inner, we should ASSERT it's not the
> outer here. Better yet, assert in SetCurrentlyScrollingLayer.
> 
> LayerTreeHostImpl::FlingScrollBegin() - Checks for either outer or inner.

I've split out the cleanup work here:
https://codereview.chromium.org/1365343002/.

[bokan, 2015-09-25 17:37:32]

Thanks, still lgtm!

[tdresser, 2015-09-29 16:46:59]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2015-09-29 16:46:59]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
jdduke@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/1306193006/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-09-29 16:47:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306193006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306193006/60001

[commit-bot: I haz the power, 2015-09-29 17:51:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-29 17:52:32]

Patchset 4 (id:??) landed as
https://crrev.com/a3b162b8f887c5cfa5c68d5bb89a89769c92d7f2
Cr-Commit-Position: refs/heads/master@{#351346}