Clear scroll anchor on all parent scrollers from ScrollAnchor::clear

third_party/WebKit/Source/core/layout/LayoutObject.h

Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index bf394922aabce42351859a70193cee730f1d28c8..0e3438341da8ace8979ecc1558fdd229fc18ad78 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -1660,9 +1660,14 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     return MutableForPainting(*this);
   }
 
-  void setIsScrollAnchorObject() { m_bitfields.setIsScrollAnchorObject(true); }
+  void setIsScrollAnchorObject(bool value) {

[skobes, 2016/10/10 21:34:35]

We want external callers to use maybeClearIsScrollAnchorObject instead of
passing false here, so I'd rather not even add the bool param.  LayoutObject can
tweak the bit directly.

[ymalik, 2016/10/11 19:54:22]

The reason I added this is because when we're clearing the scroll anchor for all
scrollers from ScrollAnchor::clear(), we already know that the bit should be set
to false since if there is any other scroller that has this LayoutObject as its
anchor, it will also be cleared.

I was trying to save a walk up the parent chain to look for other references.

I am not sure what's a better trade-off, so I have made the changes you
suggested in the comment below.

Also, I think we're already doing unnecessary walks from
LayoutObject::willBeRemovedFromTree because we have the following call graph
findReferencingScrollAnchors(Clear)->ScrollAnchor::notifyRemoved->ScrollAnchor::clear()->findReferencingScrollAnchors(DontClear).

[skobes, 2016/10/11 20:51:40]

Ah, good point.  If ScrollAnchor is doing the clear-all-ancestors walk, we do
want a LayoutObject API for "unconditional" bit-clearing.

How about:
  enum ClearIsScrollAnchorObjectBehavior { IfNotReferenced, Unconditional };
  // If you pass Unconditional, you are responsible for ensuring that
  // no ScrollAnchors reference this LayoutObject.
  clearIsScrollAnchorObject(ClearIsScrollAnchorObjectBehavior =
IfNotReferenced);

Then ScrollAnchor::clear can use Unconditional to avoid the unnecessary
find-referencing walk at each step.
Currently willBeRemovedFromTree clears the bit on itself before calling
findReferencingScrollAnchors, specifically to avoid this problem.

[ymalik, 2016/10/11 22:52:57]

Done. I used a bool instead of enum because its easier to use in
ScrollAnchor::clear instead of having a long type as a parameter. Its also a
binary and I don't expect it to grow in size.

+    m_bitfields.setIsScrollAnchorObject(value);
+  }
   // Clears the IsScrollAnchorObject bit, unless any ScrollAnchor still refers to us.
   void maybeClearIsScrollAnchorObject();
+  // Clears the scroll anchor on all scrollableAreas that are in the ancestor
+  // chain of this LayoutObject.
+  void clearScrollAnchors();

[skobes, 2016/10/10 21:34:35]

Sharing the tree-walking code is nice but I'm not sure this API is a good fit
for LayoutObject.  The call graph SA::clear() -> LO::clearScrollAnchors() ->
SA::releaseAnchor() is a confusing division of labor.

Could we put the logic for clearing ancestor ScrollAnchors in ScrollAnchor, and
limit LayoutObject's responsibilities to managing the IsScrollAnchorObject bit
and tree-removal side effects?

[ymalik, 2016/10/11 19:54:22]

Done.

 
   bool scrollAnchorDisablingStyleChanged() {
     return m_bitfields.scrollAnchorDisablingStyleChanged();