Made top controls work with virtual viewport pinch-to-zoom. (Chromium)

Made top controls work with virtual viewport pinch-to-zoom. (Chromium)

This change adjusts the scroll bubbling logic in LayerTreeHostImpl so
that outer viewport layer scrolls affect the top controls. Additionally,
the outer viewport's size is adjusted to match the inner viewport's
aspect ratio until a resize occurs.

Blink side: https://codereview.chromium.org/643473002
BUG=364109
Committed: https://crrev.com/ef97146f36e0e31632be0f7defebf2da4d72d360
Cr-Commit-Position: refs/heads/master@{#299384}

[bokan, 2014-10-08 23:03:54]

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

[bokan, 2014-10-08 23:03:55]

ptal.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2542:
layer_impl->ScrollbarParametersDidChange(false);
I think this is unneeded, it gets called when the layer bounds are changed.

[aelias_OOO_until_Jul13, 2014-10-09 03:51:36]

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

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1706: ViewportAnchor
anchor(InnerViewportScrollLayer(),
Hmm, could you describe the clamping problem that led you to create this
anchoring machinery?  It seems complicated and I'm wondering if we could find a
better way.  But I'd need to understand the problem space better first.

[bokan, 2014-10-09 13:30:13]

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

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1706: ViewportAnchor
anchor(InnerViewportScrollLayer(),
On 2014/10/09 03:51:36, aelias wrote:
> Hmm, could you describe the clamping problem that led you to create this
> anchoring machinery?  It seems complicated and I'm wondering if we could find
a
> better way.  But I'd need to understand the problem space better first.

These issues occur when the viewports are fully scrolled, changing their bounds
may place them outside of MaxScrollOffset. There's two cases to consider:

The top controls hide - both viewports increase in size. The inner viewport
should remain bounded by the outer, but the outer viewport getting larger means
that it'll be scrolled up to stay within the document bounds. The anchor moves
the inner viewport down so we stay at the bottom of the document.

Top controls show - both viewports decrease in size. When you're zoomed in, the
outer viewport will shrink by more than the inner viewport so it'll push the
inner viewport up. In this case, the anchor will move the outer viewport down to
compensate.

I was previously applying the counter scrolls explicitly in this method but it
ended up looking much messier. IMO, the anchor is actually quite simple, if a
little verbose.

[aelias_OOO_until_Jul13, 2014-10-09 20:55:13]

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

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:95: inner_->ScrollBy(gfx::Vector2dF());
Please call LayerImpl::ClampScrollToMaxScrollOffset here instead.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:98: if (outer_) {
These if (outer_) statements are mainly what makes this look complicated.  Are
those for the old path without pinch viewport?  Can we just not call this method
in that case according to the setting (which we can do if this is no longer a
destructor).

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:115: gfx::ScrollOffset
viewportInContentCoordinates_;
nit: camel-case not Chromium style

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1706: ViewportAnchor
anchor(InnerViewportScrollLayer(),
On 2014/10/09 at 13:30:13, bokan wrote:
> These issues occur when the viewports are fully scrolled, changing their
bounds may place them outside of MaxScrollOffset.

OK, seems alright.  Looks like an unavoidable consequence of making the outer
viewport aspect ratio always match the inner.  This anchoring could be avoided
by changing the sizing policy when pinched, but upon further thought that would
just lead to even more messy problems particularly with the synchronization with
Blink, so this is probably the best approach.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2572: if (!top_controls_manager_ ||
!CurrentlyScrollingLayer())
!CurrentlyScrollingLayer() condition is unnecessary, please omit or DCHECK it.

[aelias_OOO_until_Jul13, 2014-10-09 20:56:16]

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

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:93: ~ViewportAnchor() {
I'd prefer this to be an explicit method call rather than a destructor since
this doesn't free resources in the usual pattern, so it would be more readable
at the calling site.

[bokan, 2014-10-09 21:51:37]

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

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:93: ~ViewportAnchor() {
On 2014/10/09 20:56:16, aelias wrote:
> I'd prefer this to be an explicit method call rather than a destructor since
> this doesn't free resources in the usual pattern, so it would be more readable
> at the calling site.

Done.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:95: inner_->ScrollBy(gfx::Vector2dF());
On 2014/10/09 20:55:13, aelias wrote:
> Please call LayerImpl::ClampScrollToMaxScrollOffset here instead.

Done.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:98: if (outer_) {
On 2014/10/09 20:55:13, aelias wrote:
> These if (outer_) statements are mainly what makes this look complicated.  Are
> those for the old path without pinch viewport?  Can we just not call this
method
> in that case according to the setting (which we can do if this is no longer a
> destructor).

Done. Doesn't even need a guard since it's below the if(!outer_container)
early-out.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:115: gfx::ScrollOffset
viewportInContentCoordinates_;
On 2014/10/09 20:55:13, aelias wrote:
> nit: camel-case not Chromium style

Done.

https://codereview.chromium.org/641873003/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2572: if (!top_controls_manager_ ||
!CurrentlyScrollingLayer())
On 2014/10/09 20:55:13, aelias wrote:
> !CurrentlyScrollingLayer() condition is unnecessary, please omit or DCHECK it.

Done.

[aelias_OOO_until_Jul13, 2014-10-09 22:59:00]

lgtm

[bokan, 2014-10-09 23:13:51]

The CQ bit was checked by bokan@chromium.org

[bokan, 2014-10-09 23:13:55]

The CQ bit was unchecked by bokan@chromium.org

[bokan, 2014-10-10 13:49:06]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-10 13:50:20]

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

[commit-bot: I haz the power, 2014-10-10 14:49:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 14:49:28]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2014-10-10 18:24:19]

bokan@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[aelias_OOO_until_Jul13, 2014-10-10 18:29:27]

aelias@chromium.org changed reviewers:
- danakj@chromium.org, enne@chromium.org

[aelias_OOO_until_Jul13, 2014-10-10 18:29:27]

[cc danakj@, enne@] FYI, in the latest patchset, bokan@ found he needed to add
BoundsForScrolling to get correct behavior in
LTHI::UpdateViewportContainerSizes.  This shows the change in with the context
of what benefits from it, which might clarify things.  We'll hold off from CQing
this a bit to give you a chance to take a look, given that this changed after my
last l gtm.

[bokan, 2014-10-10 18:33:29]

bokan@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[danakj, 2014-10-10 18:47:16]

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:774: // TODO(aelias): Convert so that bounds returns
SizeF.
remove

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:776: return gfx::ToCeiledSize(gfx::SizeF(bounds_.width()
+ bounds_delta_.x(),
can you snap the delta to an int here and add that to the bounds_ instead of
doing the addition with floats?

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:780: gfx::SizeF LayerImpl::BoundsForScrolling() const {
This is much less scary than changing bounds(), thanks.

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:78: namespace {
you could put this in the anon namespace we already have on line 118

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:81: // it and resets to the same location on
destruction.
i don't see a destructor, comment is wrong?

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:85: : inner_(inner_scroll)
this isn't formatted right is it?

[aelias_OOO_until_Jul13, 2014-10-10 18:52:48]

For consistency, could you also change LayerImpl::SetScrollbarPosition and
LayerTreeImpl::ScrollableViewportSize to use BoundsForScrolling?

[bokan, 2014-10-10 19:25:55]

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:774: // TODO(aelias): Convert so that bounds returns
SizeF.
On 2014/10/10 18:47:16, danakj wrote:
> remove

Done.

https://codereview.chromium.org/641873003/diff/440001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:776: return gfx::ToCeiledSize(gfx::SizeF(bounds_.width()
+ bounds_delta_.x(),
On 2014/10/10 18:47:15, danakj wrote:
> can you snap the delta to an int here and add that to the bounds_ instead of
> doing the addition with floats?

Done.

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:78: namespace {
On 2014/10/10 18:47:16, danakj wrote:
> you could put this in the anon namespace we already have on line 118

Done.

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:81: // it and resets to the same location on
destruction.
On 2014/10/10 18:47:16, danakj wrote:
> i don't see a destructor, comment is wrong?

Doh! Good catch.

https://codereview.chromium.org/641873003/diff/440001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:85: : inner_(inner_scroll)
On 2014/10/10 18:47:16, danakj wrote:
> this isn't formatted right is it?

Blinkism, thanks.

[bokan, 2014-10-10 19:26:17]

On 2014/10/10 18:52:48, aelias wrote:
> For consistency, could you also change LayerImpl::SetScrollbarPosition and
> LayerTreeImpl::ScrollableViewportSize to use BoundsForScrolling?

Done.

[danakj, 2014-10-10 19:52:03]

Thanks, I think this LGTM, but I'd like enne@ to please look at it too.

https://codereview.chromium.org/641873003/diff/560001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/641873003/diff/560001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:368: // Like bounds() but doesn't snap to int.
Can you also mention that this is lossy for large layers so people will
hopefully thing about that when using it?

[bokan, 2014-10-10 19:55:58]

https://codereview.chromium.org/641873003/diff/560001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/641873003/diff/560001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:368: // Like bounds() but doesn't snap to int.
On 2014/10/10 19:52:02, danakj wrote:
> Can you also mention that this is lossy for large layers so people will
> hopefully thing about that when using it?

Done.

[bokan, 2014-10-13 18:16:23]

Hi enne@, could you PTAL? Thanks!

[enne (OOO), 2014-10-13 21:30:24]

lgtm, thanks!

[bokan, 2014-10-13 21:43:22]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-13 21:44:56]

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

[commit-bot: I haz the power, 2014-10-13 22:58:40]

Committed patchset #8 (id:650001)

[commit-bot: I haz the power, 2014-10-13 22:59:21]

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