Fix bottom/right fixed-pos with non-overlay scrollbars.

cc/trees/layer_tree_impl.cc

Index: cc/trees/layer_tree_impl.cc
diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc
index d69e7ff2e863c4410de1242e36c74666c2a2af52..93ad0a79b18870ce553fdcaa99d467cc86e9df94 100644
--- a/cc/trees/layer_tree_impl.cc
+++ b/cc/trees/layer_tree_impl.cc
@@ -143,11 +143,43 @@ LayerImpl* LayerTreeImpl::RootContainerLayer() const {
   return root_scroll_layer_ ? root_scroll_layer_->parent() : NULL;
 }
 
+LayerImpl* LayerTreeImpl::ViewportLayer() const {
+  // Return the first ancestor of the root container layer which has
+  // nonzero bounds.
+  LayerImpl* root_container = RootContainerLayer();
+  if (!root_container)
+    return NULL;
+
+  LayerImpl* ancestor = root_container->parent();
+  while (ancestor) {
+    if (!ancestor->bounds().IsEmpty())
+      return ancestor;
+    ancestor = ancestor->parent();
+  }
+  return NULL;
+}
+
 LayerImpl* LayerTreeImpl::CurrentlyScrollingLayer() const {
   DCHECK(IsActiveTree());
   return currently_scrolling_layer_;
 }
 
+int LayerTreeImpl::NonOverlayScrollbarThickness(
+    ScrollbarOrientation orientation) const {
+  LayerImpl* viewport = ViewportLayer();
+  LayerImpl* root_container = RootContainerLayer();
+  if (!viewport || !root_container)
+    return 0;
+
+  DCHECK_GE(viewport->bounds().width(), root_container->bounds().width());
+  DCHECK_GE(viewport->bounds().height(), root_container->bounds().height());
+
+  if (orientation == VERTICAL)
+    return viewport->bounds().width() - root_container->bounds().width();
+  else
+    return viewport->bounds().height() - root_container->bounds().height();
+}
+
 void LayerTreeImpl::SetCurrentlyScrollingLayer(LayerImpl* layer) {
   if (currently_scrolling_layer_ == layer)
     return;
@@ -217,12 +249,8 @@ void LayerTreeImpl::UpdateMaxScrollOffset() {
 
   // The scrollable viewport size is based on device viewport instead of Blink's
   // container layer, so we need to adjust for non-overlay scrollbars.
-  ScrollbarLayerImpl* horiz = root_scroll->horizontal_scrollbar_layer();
-  ScrollbarLayerImpl* vertical = root_scroll->vertical_scrollbar_layer();
-  if (horiz && !horiz->is_overlay_scrollbar())
-    max_scroll.set_y(max_scroll.y() + horiz->thumb_thickness());
-  if (vertical && !vertical->is_overlay_scrollbar())
-    max_scroll.set_x(max_scroll.x() + vertical->thumb_thickness());
+  max_scroll.set_x(max_scroll.x() + NonOverlayScrollbarThickness(VERTICAL));

[enne (OOO), 2013/07/30 02:32:17]

This code seems like it'd be cleaner if it set
RenderLayerCompositor::m_scrollLayer's size to the right size rather than making
assumptions from other layers.

The advantage of that is for me is that we could stop finding the first child of
the scrollable layer to guess its bounds, and then we could avoid workarounds
like https://codereview.chromium.org/14221010/.

[aelias_OOO_until_Jul13, 2013/07/30 02:52:00]

OK, I'll try that.  It will be useful for root overflow:hidden support anyway. 
Note though that this will include the scrollbar size in ScrollableSize and you
objected to those semantics last time (though I think it's fine).

[enne (OOO), 2013/07/30 03:44:47]

Oh, I see what you're getting at.  What would make sense to me would be if
ScrollableSize were the size of the scrolling contents and
ScrollableViewportSize were the size of the viewport where content can appear
(and is shrunk for non-overlay scrollbars, as they logically clip the view).

[aelias_OOO_until_Jul13, 2013/07/30 04:06:13]

That actually is not correct.  In your page scale doc, non-overlay scrollbars
lie below the page scale layer.  They are in the same space as the contents but
not the inner viewport.  (Put another way, because the scrollbars scale up, they
take up a variable amount of the inner viewport that isn't known in advance.)

+  max_scroll.set_y(max_scroll.y() + NonOverlayScrollbarThickness(HORIZONTAL));
 
   // The viewport may be larger than the contents in some cases, such as
   // having a vertical scrollbar but no horizontal overflow.
@@ -258,7 +286,7 @@ void LayerTreeImpl::UpdateSolidColorScrollbars() {
 }
 
 void LayerTreeImpl::UpdateDrawProperties() {
-  if (IsActiveTree() && RootScrollLayer() && RootContainerLayer())
+  if (IsActiveTree())
     UpdateRootScrollLayerSizeDelta();
 
   if (settings().solid_color_scrollbars &&
@@ -526,22 +554,25 @@ void LayerTreeImpl::SetRootLayerScrollOffsetDelegate(
 }
 
 void LayerTreeImpl::UpdateRootScrollLayerSizeDelta() {
-  LayerImpl* root_scroll = RootScrollLayer();
-  LayerImpl* root_container = RootContainerLayer();
-  DCHECK(root_scroll);
-  DCHECK(root_container);
-  DCHECK(IsActiveTree());
-
-  gfx::Vector2dF scrollable_viewport_size =
-      gfx::RectF(ScrollableViewportSize()).bottom_right() - gfx::PointF();
-
-  gfx::Vector2dF original_viewport_size =
-      gfx::RectF(root_container->bounds()).bottom_right() -
-      gfx::PointF();
-  original_viewport_size.Scale(1 / page_scale_factor());
+  LayerImpl* viewport_layer = ViewportLayer();
+  if (!viewport_layer)
+    return;
 
-  root_scroll->SetFixedContainerSizeDelta(
-      scrollable_viewport_size - original_viewport_size);
+  DCHECK(IsActiveTree());
+  DCHECK(root_scroll_layer_);
+
+  // This corrects fixed-position elements for 1) dynamic viewport size changes
+  // caused by things like URL-bar hiding, and 2) impl-side page scale delta.
+  gfx::SizeF true_viewport_size =
+      gfx::ScaleSize(layer_tree_host_impl_->VisibleViewportSize(),
+                     1 / total_page_scale_factor());
+  gfx::SizeF blink_perceived_viewport_size =
+      gfx::ScaleSize(viewport_layer->bounds(), 1 / page_scale_factor());

[enne (OOO), 2013/07/30 02:32:17]

Just to make sure I'm reading this correctly, ViewportLayer() here is intending
to return the overflow controls host, which includes the non-overlay scrollbars
in its bounds?

It seems to me like you would want to have a bottom fixed pos element be flush
with the viewport *not* containing the non-overlay scrollbars, which is not what
I see this code doing.

Can you help me understand what's going on here?

[aelias_OOO_until_Jul13, 2013/07/30 02:52:00]

It doesn't matter because it's a delta.  This is comparing two viewports that
include the non-overlay scrollbar size.  Equivalently, I could instead compare
two viewports that exclude the non-overlay scrollbars (which would involve
explicitly subtracting the scrollbar size from true_viewport_size).  I did it
this way because it's less lines of code.

[enne (OOO), 2013/07/30 03:44:47]

I worked through an example, and I'll buy that explanation.  Thanks.

+  gfx::Vector2dF delta(
+      true_viewport_size.width() - blink_perceived_viewport_size.width(),
+      true_viewport_size.height() - blink_perceived_viewport_size.height());
+
+  root_scroll_layer_->SetFixedContainerSizeDelta(delta);
 }
 
 void LayerTreeImpl::SetLatencyInfo(const ui::LatencyInfo& latency_info) {