Made fling gestures always bubble between inner and outer viewports.

Made fling gestures always bubble between inner and outer viewports.

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

[danakj, 2014-01-31 19:18:45]

This change should come with a test.​

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bokan, 2014-02-07 20:51:04]

Added test, PTAL

[wjmaclean, 2014-02-10 14:31:47]

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:587:
EXPECT_VECTOR_EQ(gfx::Vector2dF(25, 25), inner_scroll->TotalScrollOffset());
In order to see the bubbling happening in a 'single fling', would it be worth
making the first scroll 20 and the second 55? That way both outer *and* inner
would change on the second fling. An additional fling (another 5?) could be
added to test the inner-only scenario?

[bokan, 2014-02-10 15:04:52]

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:587:
EXPECT_VECTOR_EQ(gfx::Vector2dF(25, 25), inner_scroll->TotalScrollOffset());
On 2014/02/10 14:31:47, wjmaclean wrote:
> In order to see the bubbling happening in a 'single fling', would it be worth
> making the first scroll 20 and the second 55? That way both outer *and* inner
> would change on the second fling. An additional fling (another 5?) could be
> added to test the inner-only scenario?

I think that each ScrollUpdate from a fling can only affect a single layer at a
time (for a given direction). That is...if the max scroll offset is 50 and we're
at 45 when we send another GSU with a delta of 30, it will consume 5 pixels of
delta and the other 25 will be discarded - the bubbling happens when the next
GSU comes in and this layer can't be scrolled anymore. That's how it seems to
work based on trying the above scenario with this test and my reading of the
ScrollBy method in LTHI.cc. In practice, I think GSUs are typically 1-2 pixel
deltas so discarding the delta isn't noticeable.

[wjmaclean, 2014-02-10 15:21:50]

On 2014/02/10 15:04:52, bokan wrote:
>
https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl_unittest.cc:587:
> EXPECT_VECTOR_EQ(gfx::Vector2dF(25, 25), inner_scroll->TotalScrollOffset());
> On 2014/02/10 14:31:47, wjmaclean wrote:
> > In order to see the bubbling happening in a 'single fling', would it be
worth
> > making the first scroll 20 and the second 55? That way both outer *and*
inner
> > would change on the second fling. An additional fling (another 5?) could be
> > added to test the inner-only scenario?
> 
> I think that each ScrollUpdate from a fling can only affect a single layer at
a
> time (for a given direction). That is...if the max scroll offset is 50 and
we're
> at 45 when we send another GSU with a delta of 30, it will consume 5 pixels of
> delta and the other 25 will be discarded - the bubbling happens when the next
> GSU comes in and this layer can't be scrolled anymore. That's how it seems to
> work based on trying the above scenario with this test and my reading of the
> ScrollBy method in LTHI.cc. In practice, I think GSUs are typically 1-2 pixel
> deltas so discarding the delta isn't noticeable.

Hmm, I forgot about the whole direction constraint. Ok, then ignore my previous
comment.

[bokan, 2014-02-19 22:31:07]

Dana, please take another look at this one too.

[danakj, 2014-02-19 22:49:24]

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:260: void
SetupVirtualViewportLayers(const gfx::Size& content_size) {
Can you make a subclass of this class to put the method there (class
LayerTreeHostImplVirtualViewportTest?) so it can live closer to the test(s) that
use it.

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:276: gfx::Size(content_size.width() /
4, content_size.height() / 4));
can you make the content_size/4 and content_size/2 also parameters of this
method, and give them nice descriptive names to make the test that uses this
more obvious why 3 25px scrolls is correct?

[bokan, 2014-02-20 16:30:48]

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:260: void
SetupVirtualViewportLayers(const gfx::Size& content_size) {
On 2014/02/19 22:49:24, danakj wrote:
> Can you make a subclass of this class to put the method there (class
> LayerTreeHostImplVirtualViewportTest?) so it can live closer to the test(s)
that
> use it.

Done.

https://codereview.chromium.org/140413007/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:276: gfx::Size(content_size.width() /
4, content_size.height() / 4));
On 2014/02/19 22:49:24, danakj wrote:
> can you make the content_size/4 and content_size/2 also parameters of this
> method, and give them nice descriptive names to make the test that uses this
> more obvious why 3 25px scrolls is correct?

Done.

[danakj, 2014-02-20 16:34:59]

Thanks, LGTM

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

https://codereview.chromium.org/140413007/diff/220001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5919: gfx::Size outer_viewport =
gfx::Size(50, 50);
nit: mind making these non-symmetrical as well as the scroll offset? It'll
increase confidence in what this is testing as we know x/y aren't being mixed up
in our code at all.

[bokan, 2014-02-20 16:42:46]

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

https://codereview.chromium.org/140413007/diff/220001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5919: gfx::Size outer_viewport =
gfx::Size(50, 50);
On 2014/02/20 16:35:00, danakj wrote:
> nit: mind making these non-symmetrical as well as the scroll offset? It'll
> increase confidence in what this is testing as we know x/y aren't being mixed
up
> in our code at all.

Done.

[bokan, 2014-02-20 16:43:33]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-02-20 16:43:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/140413007/270001

[commit-bot: I haz the power, 2014-02-20 20:19:16]

Change committed as 252363