Unify the calculation of main thread offset of sticky element

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 f099cdce40417819ed11a11f133cdf54b503a75f..226e0fce0636c519af4a37691cf6a7574cc76077 100644
--- a/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "core/page/scrolling/StickyPositionScrollingConstraints.h"
+#include "core/paint/PaintLayer.h"
 
 namespace blink {
 
@@ -88,4 +89,17 @@ FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset(
   return sticky_offset;
 }
 
+FloatSize
+StickyPositionScrollingConstraints::GetPositionRelativeToScrollAncestor(
+    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();
+  }
+  return total_sticky_box_sticky_offset_ -

[chrishtr, 2017/06/05 18:58:33]

Could you explain why this logic is correct? I'm not sure
I recall what total_sticky_box_sticky_offset_ and 
nearest_sticky_box_shifting_sticky_box_constraints_offset are
exactly. (and the comments are still not there for this file. :))

[yigu, 2017/06/05 21:03:49]

We do the subtraction because total_sticky_box_sticky_offset is calculated by
ancestor_sticky_box_offset + sticky_offset. The subtraction gives us the
sticky_offset we need.
Alternatively, we could cache the sticky_offset upon calculation as
sticky_offset_ and pass it to cc as what we did with
"main_thread_sticky_offset". The downside of it is we are adding a variable to
store a value which we could compute from existing cached values.
WDYT?

[trchen, 2017/06/05 22:43:11]

+1 We need more comments in the headers. It took me a while to read through the
relevant code and label each variables in my mental map. :)

Following is my understanding of them. Feel free to rephrase/make
correction/copy them to comments.

total_sticky_box_sticky_offset_ : The accumulated layout location adjustment
applied to a sticky box, up to its containing block.

nearest_sticky_box_shifting_sticky_box_ : The nearest ancestor in location
container chain that is sticky, up to its containing block. Can be nullptr if
there is no such ancestor.

total_containing_block_sticky_offset_ : The accumulated layout location
adjustment applied to a sticky box, up to its enclosing overflow clip.
I think it is okay to use the derived value than caching it, but the naming
could be clearer. It is definitely not "position relative to scroll ancestor".
How about something like GetPreviouslyComputedStickyOffset? Also documenting
that this function is only safe to call if ComputeStickyOffset has been invoked
previously (better if DCHECK can be added, but that seems to be complicated).

[yigu, 2017/06/06 19:33:18]

I've updated the function name.

+         nearest_sticky_box_shifting_sticky_box_constraints_offset;
+}
+
 }  // namespace blink