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

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

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 1642 matching lines @@
     friend class LayoutObject;
     MutableForPainting(const LayoutObject& layoutObject)
         : m_layoutObject(const_cast<LayoutObject&>(layoutObject)) {}
 
     LayoutObject& m_layoutObject;
   };
   MutableForPainting getMutableForPainting() const {
     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();
   }
   void setScrollAnchorDisablingStyleChanged(bool changed) {
     m_bitfields.setScrollAnchorDisablingStyleChanged(changed);
   }
 
   void clearChildNeedsOverflowRecalcAfterStyleChange() {
     m_bitfields.setChildNeedsOverflowRecalcAfterStyleChange(false);
@@ skipping 815 matching lines @@
 CORE_EXPORT void showLineTree(const blink::LayoutObject*);
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1);
 // We don't make object2 an optional parameter so that showLayoutTree
 // can be called from gdb easily.
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1,
                                 const blink::LayoutObject* object2);
 
 #endif
 
 #endif  // LayoutObject_h