Move viewport scrolling logic into separate class

Move viewport scrolling logic into separate class

The viewport is made up of two scrolling layers now, the inner
and outer viewport scroll layers. These layers do not follow the
usual scroll bubbling logic since they're supposed to appear as
one viewport to the user (i.e. we should rail). Forcing it to
be scrolled via LayerTreeHostImpl::ScrollBy caused that method to
become increasingly complex.

This patch creates a Viewport class that allows LTHI::ScrollBy to
call Viewport::ScrollBy and scroll the viewport as a unit. I've
tried to simplify the logic in both these methods as much as I
could without changing behavior.

BUG=443724
Committed: https://crrev.com/aa27483190bdfbe34189e2d39c355dfda131fa8a
Cr-Commit-Position: refs/heads/master@{#322268}

[bokan, 2015-03-05 22:55:50]

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

[bokan, 2015-03-05 22:58:31]

Hi Alexandre,

As prep work for crbug.com/443724 I'm looking to clean up some of the viewport
scrolling code in CC. It's not yet ready for review and I still need to nail
down some failing scroll bubbling tests but I thought I'd ask for feedback
sooner rather than later. Does this look ok in principle to you?

[aelias_OOO_until_Jul13, 2015-03-06 04:19:37]

Looks fine in principle, seems to clarify LTHI::ScrollBy a lot.  In general I'd
like to see us gradually move all scrolling-related stuff into a tree of
structures externalized from LTHI/LTH and this is a step in that direction.

https://codereview.chromium.org/986443003/diff/1/cc/layers/viewport.h
File cc/layers/viewport.h (right):

https://codereview.chromium.org/986443003/diff/1/cc/layers/viewport.h#newcode54
cc/layers/viewport.h:54: LayerImpl* inner_scroll_layer_;
I'd rather this call out to the active tree every time to avoid duplicate state
we need to sync around.

[bokan, 2015-03-25 18:23:44]

Ready for review, this patch is a relocation of logic, it shouldn't have any
production changes

[aelias_OOO_until_Jul13, 2015-03-25 19:56:07]

Looks like a nice cleanup, mostly minor comments below.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
nit: 2015

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:41: if (OuterScrollLayer()) {
This if statement here seems weird.  The nonexistence of the outer viewport
seems like a situation that only exists during initialization (or maybe
something found only in unit tests).  I'd suggest either removing it and
changing the tests, or bailing out on a greater amount of logic than just the
one ScrollLayer call.

Would be fine to delay this change to another patch if this is here for patch
no-opness reasons.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:48: if
(!gfx::ToRoundedVector2d(pending_content_delta).IsZero()) {
This looks questionably useful, please fix or add a TODO to consider removing
this (to preserve no-opness of this change).

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h
File cc/layers/viewport.h (right):

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
Nit: 2015

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:9: #include "base/memory/weak_ptr.h"
This looks unused, please delete

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:11: #include "ui/gfx/geometry/size.h"
This looks unused, please delete

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:18: // Encapsulates the Layers that comprise the viewport.
The "viewport" is made up
It would be more accurate to write "Encapsulates gesture handling logic on the
layers"

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:28: bool did_scroll_top_controls;
I think in this struct, it would be more natural to contain the amount that the
top controls was scrolled.  Then the caller in LTHI is free to only make use of
whether it's != 0 for now.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:36: const gfx::Point& viewport_point,
Indent wrong, please run "git cl format"

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:55: LayerImpl* inner_scroll_layer_;
Looks like you deleted all use of these but not the fields themselves.

[bokan, 2015-03-25 20:38:35]

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
On 2015/03/25 19:56:06, aelias wrote:
> nit: 2015

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:41: if (OuterScrollLayer()) {
On 2015/03/25 19:56:07, aelias wrote:
> This if statement here seems weird.  The nonexistence of the outer viewport
> seems like a situation that only exists during initialization (or maybe
> something found only in unit tests).  I'd suggest either removing it and
> changing the tests, or bailing out on a greater amount of logic than just the
> one ScrollLayer call.
> 
> Would be fine to delay this change to another patch if this is here for patch
> no-opness reasons.

I think I've run into real-world cases that don't have an outer scroll layer,
though I don't remember what it was now. I can investigate but definitely in a
separate CL (I'll take a look when removing virtual-viewport-enabled forks from
CC).

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.cc#ne...
cc/layers/viewport.cc:48: if
(!gfx::ToRoundedVector2d(pending_content_delta).IsZero()) {
On 2015/03/25 19:56:07, aelias wrote:
> This looks questionably useful, please fix or add a TODO to consider removing
> this (to preserve no-opness of this change).

I thought so too but removing it caused the viewport to slide around a bit when
moving top controls. This is probably masking some other issue, I left a TODO.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h
File cc/layers/viewport.h (right):

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
On 2015/03/25 19:56:07, aelias wrote:
> Nit: 2015

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:9: #include "base/memory/weak_ptr.h"
On 2015/03/25 19:56:07, aelias wrote:
> This looks unused, please delete

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:11: #include "ui/gfx/geometry/size.h"
On 2015/03/25 19:56:07, aelias wrote:
> This looks unused, please delete

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:18: // Encapsulates the Layers that comprise the viewport.
The "viewport" is made up
On 2015/03/25 19:56:07, aelias wrote:
> It would be more accurate to write "Encapsulates gesture handling logic on the
> layers"

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:28: bool did_scroll_top_controls;
On 2015/03/25 19:56:07, aelias wrote:
> I think in this struct, it would be more natural to contain the amount that
the
> top controls was scrolled.  Then the caller in LTHI is free to only make use
of
> whether it's != 0 for now.

Done.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:36: const gfx::Point& viewport_point,
On 2015/03/25 19:56:07, aelias wrote:
> Indent wrong, please run "git cl format"

It seems to do nothing for me, maybe because I've already committed...Done
manually though.

https://codereview.chromium.org/986443003/diff/80001/cc/layers/viewport.h#new...
cc/layers/viewport.h:55: LayerImpl* inner_scroll_layer_;
On 2015/03/25 19:56:07, aelias wrote:
> Looks like you deleted all use of these but not the fields themselves.

Done.

[aelias_OOO_until_Jul13, 2015-03-25 21:07:25]

lgtm

[bokan, 2015-03-25 21:25:00]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-03-25 21:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/986443003/100001

[commit-bot: I haz the power, 2015-03-26 00:10:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-26 00:11:37]

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