Correct for enclosing layers scroll position in sticky_data->main_thread_offset

cc/layers/layer.cc

Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 2b4566a539e0545d7e7c843b7869c66447dd4e50..590aa53fb7ece1819e9830fe149defe47e2b60f0 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -29,6 +29,7 @@
 #include "cc/trees/layer_tree_impl.h"
 #include "cc/trees/mutable_properties.h"
 #include "cc/trees/mutator_host.h"
+#include "cc/trees/scroll_node.h"
 #include "cc/trees/transform_node.h"
 #include "third_party/skia/include/core/SkImageFilter.h"
 #include "ui/gfx/geometry/rect_conversions.h"
@@ -593,10 +594,26 @@ void Layer::SetPosition(const gfx::PointF& position) {
       StickyPositionNodeData* sticky_data =
           property_trees->transform_tree.StickyPositionData(
               transform_tree_index());
+      gfx::Point sticky_box_offset(
+          sticky_data->constraints.parent_relative_sticky_box_offset);
+
+      // The sticky box offset calculated in CompositedLayerMapping must be
+      // adjusted for the enclosing layer's scroll position.
+      ScrollNode* scroll_ancestor =
+          property_trees->scroll_tree.Node(scroll_tree_index());
+      if (scroll_ancestor->owning_layer_id == id()) {
+        // We are a scroller, so we need our parent's scroll_ancestor.

[flackr, 2017/02/16 22:23:59]

Can't we always get the node from the parent?

[smcgruer, 2017/02/17 13:55:31]

Yes, of course. Done.

+        scroll_ancestor =
+            property_trees->scroll_tree.Node(parent()->scroll_tree_index());
+      }
+      if (parent()->id() != scroll_ancestor->owning_layer_id) {
+        sticky_box_offset -= gfx::ScrollOffsetToFlooredVector2d(

[flackr, 2017/02/16 22:23:59]

Do we know that this floored scroll offset matches the main thread offset? There
might be a similar bug to https://crbug.com/681951 here.

I wonder if it might be better to adjust what we send for the parent relative
sticky box offset on the blink side since we will have consistent scroll
snapping there.

[smcgruer, 2017/02/17 13:55:31]

So the |enclosingLayerOffset| that is calculated blink-side is a LayoutPoint
which is an integer (as is location()). It is added to |stickyBoxOffset| which
is a FloatPoint, but the float here comes from the fact that
scrollContainerRelativeStickyBoxRect() is a FloatRect.

I think we could try to do it blink side if it would feel better, but the offset
from main for scroll (via location()) is coming in as an integer. (FYI the
scroll offset in blink-side updateLayerPosition is
"containingLayer->layoutBox()->scrolledContentOffset();" which is an IntPoint
too.)

[flackr, 2017/02/21 00:41:30]

This definitely belongs blink side. If there are cases where blink and cc
disagree about the scroll position (i.e. https://crbug.com/681951) we want to
use the one that blink is using when it sets the layer position of the sticky
layer.

[smcgruer, 2017/02/21 19:17:21]

Done.

+            property_trees->scroll_tree.current_scroll_offset(
+                scroll_ancestor->owning_layer_id));
+      }
+
       sticky_data->main_thread_offset =
-          position.OffsetFromOrigin() -
-          sticky_data->constraints.parent_relative_sticky_box_offset
-              .OffsetFromOrigin();
+          position.OffsetFromOrigin() - sticky_box_offset.OffsetFromOrigin();
     }
     transform_node->needs_local_transform_update = true;
     transform_node->transform_changed = true;