Slightly refactor StickyPositionScrollingConstraints API and add documentation

third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp

Index: third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
diff --git a/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp b/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
index 61150ecbbc4485b164011349d4eae3a78b547eee..8297c0982bf5e69a4109395e0bd9a63669140330 100644
--- a/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
@@ -9,28 +9,38 @@ namespace blink {
 
 FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset(
     const FloatRect& viewport_rect,
-    const StickyPositionScrollingConstraints* ancestor_sticky_box_constraints,
-    const StickyPositionScrollingConstraints*
-        ancestor_containing_block_constraints) {
-  // Adjust the constraint rect locations based on our ancestor sticky elements
-  // These adjustments are necessary to avoid double offsetting in the case of
-  // nested sticky elements.
-  FloatSize ancestor_sticky_box_offset =
-      ancestor_sticky_box_constraints
-          ? ancestor_sticky_box_constraints->GetTotalStickyBoxStickyOffset()
-          : FloatSize();
-  FloatSize ancestor_containing_block_offset =
-      ancestor_containing_block_constraints
-          ? ancestor_containing_block_constraints
-                ->GetTotalContainingBlockStickyOffset()
-          : FloatSize();
+    const StickyConstraintsMap& constraints_map) {
   FloatRect sticky_box_rect = scroll_container_relative_sticky_box_rect_;
   FloatRect containing_block_rect =
       scroll_container_relative_containing_block_rect_;
+  FloatSize ancestor_sticky_box_offset =
+      AncestorStickyBoxOffset(constraints_map);
+  FloatSize ancestor_containing_block_offset =
+      AncestorContainingBlockOffset(constraints_map);
+
+  // Adjust the cached rect locations for any sticky ancestor elements. The
+  // sticky offset applied to those ancestors affects us as follows:
+  //
+  //   1. |nearest_sticky_layer_shifting_sticky_box_| is a sticky layer between
+  //      ourselves and our containing block, e.g. an inline parent. As such,

[smcgruer, 2017/06/26 19:18:56]

I think this is inaccurate, an inline parent is insufficient here. If you look
at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. It's
location_container is an anonymous div whose Parent() is the scroller itself,
not the outer inline sticky1 element.

[flackr, 2017/06/29 15:24:10]

Yeah so it needs to be a sticky layer that is not the containing block for the
descendant but does shift it's location (i.e. location container). Maybe tables
are the only real instance of this? Also, this sounds related to
https://crbug.com/695481

[smcgruer, 2017/06/29 18:51:46]

Nested inline appears to work, so will likely use that as an example (not
rewritten yet). Also https://crbug.com/695481 is not nested sticky, did you
think it related in some other way?

[flackr, 2017/06/30 02:48:35]

Yes, I was thinking it might be related if the reason the block elements aren't
moving might be because their location container isn't the inline.

[smcgruer, 2017/07/07 13:51:57]

Fixed the comment.

+  //      it shifts only the sticky_box_rect, and not the containing_block_rect.
+  //   2. |nearest_sticky_layer_shifting_containing_block_| is a sticky layer
+  //      between our containing block (inclusive) and our scroll ancestor
+  //      (exclusive). As such, it shifts both the sticky_box_rect and the
+  //      containing_block_rect.
+  //
+  // Note that this calculation assumes that |ComputeStickyOffset| is being
+  // called top down, e.g. it has been called on any ancestors we have before
+  // being called on us.
   sticky_box_rect.Move(ancestor_sticky_box_offset +
                        ancestor_containing_block_offset);
   containing_block_rect.Move(ancestor_containing_block_offset);
 
+  // We now attempt to shift sticky_box_rect to obey the specified sticky
+  // constraints, whilst always staying within our containing block. This
+  // shifting produces the final sticky offset below.
+  //
+  // As per the spec, 'left' overrides 'right' and 'top' overrides 'bottom'.
   FloatRect box_rect = sticky_box_rect;
 
   if (HasAnchorEdge(kAnchorEdgeRight)) {
@@ -81,6 +91,13 @@ FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset(
 
   FloatSize sticky_offset = box_rect.Location() - sticky_box_rect.Location();
 
+  // For performance, we cache our accumulated sticky offsets for any descendant
+  // sticky to use in it's own |ComputeStickyOffset| call. If these were not
+  // cached, one would instead need to call |ComputeStickyOffset| on every
+  // sticky ancestor in turn, accumulating the results.
+
+  // TODO(smcgruer): Document why these are calculated like this and why the two

[smcgruer, 2017/06/26 19:18:56]

As per our IM discussion, I am still trying to explain (and understand!) why we
add 'ancestor_sticky_box_offset' for 'total_containing_block_sticky_offset_',
especially given that no tests fail if I remove that.

So far it is not easy, and I am beginning to wonder if it was predominantly
table-related which is no longer reproducible since we dont support <tr> and
<thead> anymore.

+  // different members are necessary.
   total_sticky_box_sticky_offset_ = ancestor_sticky_box_offset + sticky_offset;
   total_containing_block_sticky_offset_ = ancestor_sticky_box_offset +
                                           ancestor_containing_block_offset +
@@ -91,14 +108,33 @@ FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset(
 
 FloatSize StickyPositionScrollingConstraints::GetOffsetForStickyPosition(
     const StickyConstraintsMap& constraints_map) const {
-  FloatSize nearest_sticky_box_shifting_sticky_box_constraints_offset;
-  if (nearest_sticky_box_shifting_sticky_box_) {
-    nearest_sticky_box_shifting_sticky_box_constraints_offset =
-        constraints_map.at(nearest_sticky_box_shifting_sticky_box_->Layer())
-            .GetTotalStickyBoxStickyOffset();
+  FloatSize nearest_sticky_layer_shifting_sticky_box_constraints_offset;
+  if (nearest_sticky_layer_shifting_sticky_box_) {
+    nearest_sticky_layer_shifting_sticky_box_constraints_offset =
+        constraints_map.at(nearest_sticky_layer_shifting_sticky_box_)
+            .total_sticky_box_sticky_offset_;
   }
   return total_sticky_box_sticky_offset_ -
-         nearest_sticky_box_shifting_sticky_box_constraints_offset;
+         nearest_sticky_layer_shifting_sticky_box_constraints_offset;
+}
+
+FloatSize StickyPositionScrollingConstraints::AncestorStickyBoxOffset(
+    const StickyConstraintsMap& constraints_map) {
+  if (!nearest_sticky_layer_shifting_sticky_box_)
+    return FloatSize();
+  DCHECK(constraints_map.Contains(nearest_sticky_layer_shifting_sticky_box_));

[flackr, 2017/06/29 15:24:10]

Is this check necessary when we use the constraints on the next line? i.e. does
it not crash when trying to access the total_sticky_box_offset_ if that isn't in
the map?

[smcgruer, 2017/06/29 18:51:46]

It's necessary. at() just calls EmptyValue() if it doesn't find anything.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/H...

+  return constraints_map.at(nearest_sticky_layer_shifting_sticky_box_)
+      .total_sticky_box_sticky_offset_;
+}
+
+FloatSize StickyPositionScrollingConstraints::AncestorContainingBlockOffset(
+    const StickyConstraintsMap& constraints_map) {
+  if (!nearest_sticky_layer_shifting_containing_block_)
+    return FloatSize();
+  DCHECK(constraints_map.Contains(
+      nearest_sticky_layer_shifting_containing_block_));
+  return constraints_map.at(nearest_sticky_layer_shifting_containing_block_)
+      .total_containing_block_sticky_offset_;
 }
 
 }  // namespace blink