Slightly refactor StickyPositionScrollingConstraints API and add documentation

third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
index 1e94646622ff44ce244bab0bf2f90b7ac1de1bc1..2ffe0fe552808acb1091a76d43157d5f4bd4a73a 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
@@ -54,28 +54,12 @@ inline bool IsOutOfFlowPositionedWithImplicitHeight(
          !child->Style()->LogicalBottom().IsAuto();
 }
 
-StickyPositionScrollingConstraints* StickyConstraintsForLayoutObject(
-    const LayoutBoxModelObject* obj,
-    const PaintLayer* ancestor_overflow_layer) {
-  if (!obj)
-    return nullptr;
-
-  PaintLayerScrollableArea* scrollable_area =
-      ancestor_overflow_layer->GetScrollableArea();
-  auto it = scrollable_area->GetStickyConstraintsMap().find(obj->Layer());
-  if (it == scrollable_area->GetStickyConstraintsMap().end())
-    return nullptr;
-
-  return &it->value;
-}
-
 // Inclusive of |from|, exclusive of |to|.
-LayoutBoxModelObject* FindFirstStickyBetween(LayoutObject* from,
-                                             LayoutObject* to) {
+PaintLayer* FindFirstStickyBetween(LayoutObject* from, LayoutObject* to) {
   LayoutObject* maybe_sticky_ancestor = from;
   while (maybe_sticky_ancestor && maybe_sticky_ancestor != to) {
     if (maybe_sticky_ancestor->Style()->HasStickyConstrainedPosition()) {
-      return ToLayoutBoxModelObject(maybe_sticky_ancestor);
+      return ToLayoutBoxModelObject(maybe_sticky_ancestor)->Layer();
     }
 
     maybe_sticky_ancestor =
@@ -998,12 +982,14 @@ void LayoutBoxModelObject::UpdateStickyPositionConstraints() const {
   //
   // The respective search ranges are [container, containingBlock) and
   // [containingBlock, scrollAncestor).
-  constraints.SetNearestStickyBoxShiftingStickyBox(
+  constraints.SetNearestStickyLayerShiftingStickyBox(
       FindFirstStickyBetween(location_container, containing_block));
   // We cannot use |scrollAncestor| here as it disregards the root
   // ancestorOverflowLayer(), which we should include.
-  constraints.SetNearestStickyBoxShiftingContainingBlock(FindFirstStickyBetween(
-      containing_block, &Layer()->AncestorOverflowLayer()->GetLayoutObject()));
+  constraints.SetNearestStickyLayerShiftingContainingBlock(
+      FindFirstStickyBetween(
+          containing_block,
+          &Layer()->AncestorOverflowLayer()->GetLayoutObject()));
 
   // We skip the right or top sticky offset if there is not enough space to
   // honor both the left/right or top/bottom offsets.
@@ -1104,27 +1090,18 @@ LayoutSize LayoutBoxModelObject::StickyPositionOffset() const {
   if (!ancestor_overflow_layer)
     return LayoutSize();
 
-  StickyPositionScrollingConstraints* constraints =
-      StickyConstraintsForLayoutObject(this, ancestor_overflow_layer);
-  if (!constraints)
+  StickyConstraintsMap& constraints_map =
+      ancestor_overflow_layer->GetScrollableArea()->GetStickyConstraintsMap();
+  auto it = constraints_map.find(Layer());
+  if (it == constraints_map.end())
     return LayoutSize();
-
-  StickyPositionScrollingConstraints* shifting_sticky_box_constraints =
-      StickyConstraintsForLayoutObject(
-          constraints->NearestStickyBoxShiftingStickyBox(),
-          ancestor_overflow_layer);
-
-  StickyPositionScrollingConstraints* shifting_containing_block_constraints =
-      StickyConstraintsForLayoutObject(
-          constraints->NearestStickyBoxShiftingContainingBlock(),
-          ancestor_overflow_layer);
+  StickyPositionScrollingConstraints* constraints = &it->value;

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

Refactoring this code made me realize how dangerous this behavior is.
StickyPositionScrollingConstraints is still being treated like a const PoD,
however we need the specific object in the hashmap to be updated when
ComputeStickyOffset is called. That only happens properly afaik because HashMap
iteration happens to be implemented that way.

I'm pretty sure we should change the map to be PaintLayer* ->
std::unique_ptr<StickyPositionScrollingConstraints>, but haven't done so here.

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

I think it's okay to modify the value being pointed to by the hashmap.

 
   // The sticky offset is physical, so we can just return the delta computed in
   // absolute coords (though it may be wrong with transforms).
   FloatRect constraining_rect = ComputeStickyConstrainingRect();
-  return LayoutSize(constraints->ComputeStickyOffset(
-      constraining_rect, shifting_sticky_box_constraints,
-      shifting_containing_block_constraints));
+  return LayoutSize(
+      constraints->ComputeStickyOffset(constraining_rect, constraints_map));
 }
 
 LayoutPoint LayoutBoxModelObject::AdjustedPositionRelativeTo(