Unify the calculation of main thread offset of sticky element

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/page/scrolling/StickyPositionScrollingConstraints.h"
+#include "core/paint/PaintLayer.h"
 
 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
@@ skipping 65 matching lines @@
   FloatSize sticky_offset = box_rect.Location() - sticky_box_rect.Location();
 
   total_sticky_box_sticky_offset_ = ancestor_sticky_box_offset + sticky_offset;
   total_containing_block_sticky_offset_ = ancestor_sticky_box_offset +
                                           ancestor_containing_block_offset +
                                           sticky_offset;
 
   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