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

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index e0d2e26149d089785fae8fb3c501d32b5147a309..01a53ae876185ab15c60ccd55a8faa3861e81761 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -75,6 +75,48 @@
 #include "ui/gfx/size_conversions.h"
 #include "ui/gfx/vector2d_conversions.h"
 
+namespace {
+
+// Small helper class that saves the current viewport location as the user sees
+// it and resets to the same location on destruction.
+class ViewportAnchor {
+ public:
+  ViewportAnchor(cc::LayerImpl* inner_scroll, cc::LayerImpl* outer_scroll)
+  : inner_(inner_scroll)
+  , outer_(outer_scroll) {
+    viewportInContentCoordinates_ = inner_->TotalScrollOffset();
+
+    if (outer_)
+      viewportInContentCoordinates_ += outer_->TotalScrollOffset();
+  }
+
+  ~ViewportAnchor() {

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

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]

Done.

+    // No-op scroll to force clamp to maxScrollOffset.
+    inner_->ScrollBy(gfx::Vector2dF());

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

Please call LayerImpl::ClampScrollToMaxScrollOffset here instead.

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

Done.

+    gfx::ScrollOffset viewport_location = inner_->TotalScrollOffset();
+
+    if (outer_) {

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

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).

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

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

+      outer_->ScrollBy(gfx::Vector2dF());
+      viewport_location += outer_->TotalScrollOffset();
+    }
+
+    gfx::Vector2dF delta =
+        viewportInContentCoordinates_.DeltaFrom(viewport_location);
+
+    if (outer_)
+      delta = outer_->ScrollBy(delta);
+
+    inner_->ScrollBy(delta);
+  }
+
+ private:
+  cc::LayerImpl* inner_;
+  cc::LayerImpl* outer_;
+  gfx::ScrollOffset viewportInContentCoordinates_;

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

nit: camel-case not Chromium style

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

Done.

+};
+
+}  // namespace
+
 namespace cc {
 namespace {
 
@@ -1653,15 +1695,39 @@ void LayerTreeHostImpl::WillBeginImplFrame(const BeginFrameArgs& args) {
   begin_impl_frame_interval_ = args.interval;
 }
 
-void LayerTreeHostImpl::UpdateInnerViewportContainerSize() {
-  LayerImpl* container_layer = active_tree_->InnerViewportContainerLayer();
-  if (!container_layer)
+void LayerTreeHostImpl::UpdateViewportContainerSizes() {
+  LayerImpl* inner_container = active_tree_->InnerViewportContainerLayer();
+  LayerImpl* outer_container = active_tree_->OuterViewportContainerLayer();
+
+  if (!inner_container || !top_controls_manager_)
     return;
 
-  if (top_controls_manager_) {
-    container_layer->SetBoundsDelta(
+  {
+    ViewportAnchor anchor(InnerViewportScrollLayer(),

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

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]

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]

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.

+                          OuterViewportScrollLayer());
+
+    // Adjust the inner viewport by shrinking/expanding the container to account
+    // for the change in top controls height since the last Resize from Blink.
+    inner_container->SetBoundsDelta(
         gfx::Vector2dF(0, active_tree_->top_controls_layout_height() -
             active_tree_->total_top_controls_content_offset()));
+
+    if (!outer_container || outer_container->bounds().IsEmpty())
+      return;
+
+    // Adjust the outer viewport container as well, since adjusting only the
+    // inner may cause its bounds to exceed those of the outer, causing scroll
+    // clamping. We adjust it so it maintains the same aspect ratio as the
+    // inner viewport.
+    float aspect_ratio = inner_container->bounds().width() /
+        inner_container->bounds().height();
+    float target_height = outer_container->bounds().width() / aspect_ratio;
+    float current_outer_height = outer_container->bounds().height() -
+        outer_container->bounds_delta().y();
+    gfx::Vector2dF delta(0, target_height - current_outer_height);
+
+    outer_container->SetBoundsDelta(delta);
+    active_tree_->InnerViewportScrollLayer()->SetBoundsDelta(delta);
   }
 }
 
@@ -1670,7 +1736,7 @@ void LayerTreeHostImpl::SetTopControlsLayoutHeight(float height) {
     return;
 
   active_tree_->set_top_controls_layout_height(height);
-  UpdateInnerViewportContainerSize();
+  UpdateViewportContainerSizes();
   SetFullRootLayerDamage();
 }
 
@@ -1791,7 +1857,7 @@ void LayerTreeHostImpl::ActivateSyncTree() {
           top_controls_manager_->top_controls_height());
     }
 
-    UpdateInnerViewportContainerSize();
+    UpdateViewportContainerSizes();
   } else {
     active_tree_->ProcessUIResourceRequestQueue();
   }
@@ -2154,7 +2220,7 @@ void LayerTreeHostImpl::SetViewportSize(const gfx::Size& device_viewport_size) {
 
   device_viewport_size_ = device_viewport_size;
 
-  UpdateInnerViewportContainerSize();
+  UpdateViewportContainerSizes();
   client_->OnCanDrawStateChanged(CanDraw());
   SetFullRootLayerDamage();
   active_tree_->set_needs_update_draw_properties();
@@ -2205,7 +2271,7 @@ const gfx::Transform& LayerTreeHostImpl::DrawTransform() const {
 }
 
 void LayerTreeHostImpl::DidChangeTopControlsPosition() {
-  UpdateInnerViewportContainerSize();
+  UpdateViewportContainerSizes();
   SetNeedsRedraw();
   SetNeedsAnimate();
   active_tree_->set_needs_update_draw_properties();
@@ -2501,6 +2567,29 @@ static gfx::Vector2dF ScrollLayerWithLocalDelta(LayerImpl* layer_impl,
   return layer_impl->ScrollDelta() - previous_delta;
 }
 
+bool LayerTreeHostImpl::ShouldTopControlsConsumeScroll(
+    const gfx::Vector2dF& scroll_delta) const {
+  if (!top_controls_manager_ || !CurrentlyScrollingLayer())

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

!CurrentlyScrollingLayer() condition is unnecessary, please omit or DCHECK it.

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

Done.

+    return false;
+
+  // Always consume if it's in the direction to show the top controls.
+  if (scroll_delta.y() < 0)
+    return true;
+
+  if (CurrentlyScrollingLayer() != InnerViewportScrollLayer() &&
+      CurrentlyScrollingLayer() != OuterViewportScrollLayer())
+    return false;
+
+  if (InnerViewportScrollLayer()->MaxScrollOffset().y() > 0)
+    return true;
+
+  if (OuterViewportScrollLayer() &&
+      OuterViewportScrollLayer()->MaxScrollOffset().y() > 0)
+    return true;
+
+  return false;
+}
+
 bool LayerTreeHostImpl::ScrollBy(const gfx::Point& viewport_point,
                                  const gfx::Vector2dF& scroll_delta) {
   TRACE_EVENT0("cc", "LayerTreeHostImpl::ScrollBy");
@@ -2512,14 +2601,8 @@ bool LayerTreeHostImpl::ScrollBy(const gfx::Point& viewport_point,
   bool did_scroll_x = false;
   bool did_scroll_y = false;
   bool did_scroll_top_controls = false;
-  // TODO(wjmaclean) Should we guard against CurrentlyScrollingLayer() == 0
-  // here?
-  bool consume_by_top_controls =
-      top_controls_manager_ &&
-      (((CurrentlyScrollingLayer() == InnerViewportScrollLayer() ||
-         CurrentlyScrollingLayer() == OuterViewportScrollLayer()) &&
-        InnerViewportScrollLayer()->MaxScrollOffset().y() > 0) ||
-       scroll_delta.y() < 0);
+
+  bool consume_by_top_controls = ShouldTopControlsConsumeScroll(scroll_delta);
 
   for (LayerImpl* layer_impl = CurrentlyScrollingLayer();
        layer_impl;
@@ -2527,23 +2610,20 @@ bool LayerTreeHostImpl::ScrollBy(const gfx::Point& viewport_point,
     if (!layer_impl->scrollable())
       continue;
 
-    if (layer_impl == InnerViewportScrollLayer()) {
-      // Only allow bubble scrolling when the scroll is in the direction to make
-      // the top controls visible.
-      gfx::Vector2dF applied_delta;
-      gfx::Vector2dF excess_delta;
+    if (layer_impl == InnerViewportScrollLayer() ||
+        layer_impl == OuterViewportScrollLayer()) {
       if (consume_by_top_controls) {
-        excess_delta = top_controls_manager_->ScrollBy(pending_delta);
-        applied_delta = pending_delta - excess_delta;
+        gfx::Vector2dF excess_delta =
+            top_controls_manager_->ScrollBy(pending_delta);
+        gfx::Vector2dF applied_delta = pending_delta - excess_delta;
         pending_delta = excess_delta;
         // Force updating of vertical adjust values if needed.
-        if (applied_delta.y() != 0) {
+        if (applied_delta.y() != 0)
           did_scroll_top_controls = true;
-          layer_impl->ScrollbarParametersDidChange(false);

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

I think this is unneeded, it gets called when the layer bounds are changed.

-        }
       }
       // Track root layer deltas for reporting overscroll.
-      unused_root_delta = pending_delta;
+      if (layer_impl == InnerViewportScrollLayer())
+        unused_root_delta = pending_delta;
     }
 
     gfx::Vector2dF applied_delta;