Make scrollable viewport size no longer depend on clip layer.

Make scrollable viewport size no longer depend on clip layer.
    
In non-solid-color mode, the scrollable viewport size calculation was
ignoring device_viewport_size and using the clip layer size instead, in
order to take into account non-overlay-scrollbars.  This was weird and
was going to behave incorrectly with desktop pinch zoom.  This patch
changes the non-overlay-scrollbar size to be added to the max scroll
offset instead.

NOTRY=true
BUG=259141
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211496

[aelias_OOO_until_Jul13, 2013-07-12 01:33:45]

Hi Enne, this will need to land first before I can land the masksToBounds
changes.  I could've just removed the "masksToBounds" condition, but I have
never been happy with this logic branching to begin with (and I think it will
cause problems for wjmaclean) so I went ahead and fixed it to what I believe is
the right way.

[enne (OOO), 2013-07-12 17:19:27]

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl.cc:1297: if (!Settings().solid_color_scrollbars &&
clip_layer &&
I didn't even know this code existed, but I am happy to see it go.

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:313: // For the purposes of scrolling, non-overlay
scrollbars are considered to
This code looks a bit suspicious to me and I need some more convincing.

Why would the scrollable size change because of scrollbars? If you have both
horizontal and vertical overflow (and thus have both scrollbars), I wouldn't
expect the scrollable size to shrink because there was a clip layer with
scrollbars above it.

[aelias_OOO_until_Jul13, 2013-07-12 18:18:23]

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:313: // For the purposes of scrolling, non-overlay
scrollbars are considered to
On 2013/07/12 17:19:27, enne wrote:
> This code looks a bit suspicious to me and I need some more convincing.
> 
> Why would the scrollable size change because of scrollbars? If you have both
> horizontal and vertical overflow (and thus have both scrollbars), I wouldn't
> expect the scrollable size to shrink because there was a clip layer with
> scrollbars above it.

Note that this code is adding to the scrollable size, not shrinking it.

There are two ways of considering the effect of non-overlay scrollbars.  They
can shrink the viewport or expand the scrollable size.  Those interpretations
are equivalent until pinch zoom comes into play.  Recall that wjmaclean's
desired UX is to make the non-overlay scrollbars scale up and to become visible
if you scroll the inner viewport to the right/bottom, but be invisible
otherwise.  This matches "expand the scrollable size" semantics.

[enne (OOO), 2013-07-12 18:30:27]

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/18202006/diff/6001/cc/trees/layer_tree_impl.c...
cc/trees/layer_tree_impl.cc:313: // For the purposes of scrolling, non-overlay
scrollbars are considered to
On 2013/07/12 18:18:23, aelias wrote:
> On 2013/07/12 17:19:27, enne wrote:
> > This code looks a bit suspicious to me and I need some more convincing.
> > 
> > Why would the scrollable size change because of scrollbars? If you have both
> > horizontal and vertical overflow (and thus have both scrollbars), I wouldn't
> > expect the scrollable size to shrink because there was a clip layer with
> > scrollbars above it.
> 
> Note that this code is adding to the scrollable size, not shrinking it.

I don't think the scroll layer size is at all related to the scrollbars, growing
or shrinking.

> There are two ways of considering the effect of non-overlay scrollbars.  They
> can shrink the viewport or expand the scrollable size.

I do not agree that expanding the scrollable size is equivalent.  I think this
would be better placed in UpdateMaxScrollOffset rather than changing the
scrollable size.

[aelias_OOO_until_Jul13, 2013-07-12 19:04:03]

On 2013/07/12 18:30:27, enne wrote:
> > There are two ways of considering the effect of non-overlay scrollbars. 
They
> > can shrink the viewport or expand the scrollable size.
> 
> I do not agree that expanding the scrollable size is equivalent.  I think this
> would be better placed in UpdateMaxScrollOffset rather than changing the
> scrollable size.

Good point, MaxScrollOffset makes sense, done.

[aelias_OOO_until_Jul13, 2013-07-12 23:43:34]

Friendly ping?  This should be ready to land.

[enne (OOO), 2013-07-12 23:48:04]

(I'm not trying to be rude, but does a 4h old code review with two comments
already from me today really need a friendly ping?  I haven't forgotten it; just
trying to write some code.../o\)

https://codereview.chromium.org/18202006/diff/17001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/18202006/diff/17001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:223: max_scroll.set_y(max_scroll.y() +
horiz->thumb_thickness());
I still don't really understand this.  You can scroll more horizontally if you
have a horizontal scrollbar?

Are you sure that Blink doesn't already do the right thing here?

[enne (OOO), 2013-07-12 23:50:30]

lgtm

https://codereview.chromium.org/18202006/diff/17001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/18202006/diff/17001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.cc:223: max_scroll.set_y(max_scroll.y() +
horiz->thumb_thickness());
Eh, sorry, misreading code late on a Friday.

I guess this all makes sense.

[aelias_OOO_until_Jul13, 2013-07-12 23:53:45]

Sorry for rushing you, I just thought you might not have noticed the last
update.  I agree your reviews have been quick, thanks :)

[commit-bot: I haz the power, 2013-07-12 23:57:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/18202006/17001

[commit-bot: I haz the power, 2013-07-13 00:04:35]

Change committed as 211496