Add a bit to not bubble scrolls to parent scrolling layers

Add a bit to not bubble scrolls to parent scrolling layers

This will be used by flings to avoid bubbling to parent layers. Also adds a
private bool to record whether we have already flung in the currentlyScrollingLayer
to avoid flinging in multiple layers in multiple scrollBy calls.

BUG=169548
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179759

[Yusuf, 2013-01-27 23:28:13]

WIP patch. I will add unit tests soon.

[jamesr, 2013-01-29 05:52:24]

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h
File cc/input_handler.h (right):

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h#newcode49
cc/input_handler.h:49: virtual bool scrollBy(const gfx::Point&, const
gfx::Vector2d&, bool shouldBubble) = 0;
why do you need another bool? you have the scroll type in scrollBegin() which
IIUC determines the bubbling behavior of the scroll sequence

[Sami, 2013-01-29 11:42:34]

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h
File cc/input_handler.h (right):

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h#newcode49
cc/input_handler.h:49: virtual bool scrollBy(const gfx::Point&, const
gfx::Vector2d&, bool shouldBubble) = 0;
On 2013/01/29 05:52:24, jamesr wrote:
> why do you need another bool? you have the scroll type in scrollBegin() which
> IIUC determines the bubbling behavior of the scroll sequence

We still want to bubble slow gesture scrolls as opposed to flings. We could add
another ScrollInputType, e.g., NonBubblingGesture so we can tell the two apart.
Alternatively it could be a boolean, although I don't think there's ever a need
to avoid bubbling mouse wheel events.

Generally a fling starts with a normal gesture scroll which then turns into a
fling when the gesture is recognized. Earlier I thought these were part of the
same scrollBegin/End transaction, but turns out we start a new one when the
fling animation kicks in. So, let's add a way to tell scrollBegin whether we
want to bubble or not and leave scrollBy as it is.

https://codereview.chromium.org/12090014/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12090014/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1319: if (shouldBubble || !m_didScrollOnce)
I'm not sure this is correct. When we're not bubbling, this seems to allow
scrollBy to bubble once up to a layer that can scroll. The next time scrollBy is
called, it terminates at the first layer that couldn't scroll.

Let's say you have a page with an overflow div that was already scrolled all the
way down. Flinging that div further down should fling the page instead. I think
with this code that wouldn't happen.

https://codereview.chromium.org/12090014/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1322: break;
Indentation seem off.

[Yusuf, 2013-01-29 18:59:21]

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h
File cc/input_handler.h (right):

https://codereview.chromium.org/12090014/diff/1/cc/input_handler.h#newcode49
cc/input_handler.h:49: virtual bool scrollBy(const gfx::Point&, const
gfx::Vector2d&, bool shouldBubble) = 0;
OK. Sorry I didn't notice that we can use scrollType for this. I will add
another state to the enum.


On 2013/01/29 11:42:34, Sami wrote:
> On 2013/01/29 05:52:24, jamesr wrote:
> > why do you need another bool? you have the scroll type in scrollBegin()
which
> > IIUC determines the bubbling behavior of the scroll sequence
> 
> We still want to bubble slow gesture scrolls as opposed to flings. We could
add
> another ScrollInputType, e.g., NonBubblingGesture so we can tell the two
apart.
> Alternatively it could be a boolean, although I don't think there's ever a
need
> to avoid bubbling mouse wheel events.
> 
> Generally a fling starts with a normal gesture scroll which then turns into a
> fling when the gesture is recognized. Earlier I thought these were part of the
> same scrollBegin/End transaction, but turns out we start a new one when the
> fling animation kicks in. So, let's add a way to tell scrollBegin whether we
> want to bubble or not and leave scrollBy as it is.

https://codereview.chromium.org/12090014/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12090014/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1319: if (shouldBubble || !m_didScrollOnce)
Yes, I think you are right. I somehow thought the current_scrolling_layer gets
updated when we bubble up here, so that we remember where we left of, but that
doesn't look like the case. I might need to keep the layer itself like we had
talked before.

On 2013/01/29 11:42:34, Sami wrote:
> I'm not sure this is correct. When we're not bubbling, this seems to allow
> scrollBy to bubble once up to a layer that can scroll. The next time scrollBy
is
> called, it terminates at the first layer that couldn't scroll.
> 
> Let's say you have a page with an overflow div that was already scrolled all
the
> way down. Flinging that div further down should fling the page instead. I
think
> with this code that wouldn't happen.

[Yusuf, 2013-01-30 01:50:35]

I took one last stab at doing this without needing to keep an extra layer. I
thought it makes sense to update the currently_scrolling_layer if we are not
going to bubble. Also added a test. PTAL

[Sami, 2013-01-30 19:28:31]

I think the logic and test both look good.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1208: m_didScrollOnce = false;
Better do this in clearCurrentlyScrollingLayer().

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1352: break;
Indent?

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1354: if (!m_shouldBubbleScrolls)
m_activeTree->set_currently_scrolling_layer(layerImpl);
You could also break here since there's no need to continue recursing, but only
after updating didScroll and m_didScrollOnce.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:330: bool m_didScrollOnce;
At the risk of bikeshedding I think this name is a bit obscure. Maybe something
like m_didLockScrollingLayer or m_didFindScrollingLayer?

[jamesr, 2013-01-30 19:32:23]

lgtm2 modulo Sami's comments

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:330: bool m_didScrollOnce;
On 2013/01/30 19:28:31, Sami wrote:
> At the risk of bikeshedding I think this name is a bit obscure. Maybe
something
> like m_didLockScrollingLayer or m_didFindScrollingLayer?

Or maybe it's time to move the transient scroll properties out to another
struct/class?

[Yusuf, 2013-01-30 21:01:01]

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1208: m_didScrollOnce = false;
On 2013/01/30 19:28:31, Sami wrote:
> Better do this in clearCurrentlyScrollingLayer().

Done.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1352: break;
On 2013/01/30 19:28:31, Sami wrote:
> Indent?

Done.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1352: break;
On 2013/01/30 19:28:31, Sami wrote:
> Indent?

Done.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1354: if (!m_shouldBubbleScrolls)
m_activeTree->set_currently_scrolling_layer(layerImpl);
On 2013/01/30 19:28:31, Sami wrote:
> You could also break here since there's no need to continue recursing, but
only
> after updating didScroll and m_didScrollOnce.

Done.

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/12090014/diff/8002/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:330: bool m_didScrollOnce;
On 2013/01/30 19:28:31, Sami wrote:
> At the risk of bikeshedding I think this name is a bit obscure. Maybe
something
> like m_didLockScrollingLayer or m_didFindScrollingLayer?

Done.

[Yusuf, 2013-01-30 21:28:10]

WebKit side patch needed after this.

https://bugs.webkit.org/show_bug.cgi?id=108372

[commit-bot: I haz the power, 2013-01-30 22:42:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusufo@chromium.org/12090014/14001

[commit-bot: I haz the power, 2013-01-31 01:29:13]

Change committed as 179759