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

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

Before this CL, a call to ScrollAnchor::clear() would clear the anchor node
on the containing scroller, and set the LayoutObject::IsScrollAnchorObject
bit on the anchor if there is no other scroll referencing it.

With this CL, a call to ScrollAnchor::clear() walks up the parent chain and
clears the scroll anchor for every scroller encountered in the walk. As a
result, the LayoutObject::IsScrollAnchorObject is always set to false.

This CL is dependent on https://codereview.chromium.org/2400723002

BUG=650017
Committed: https://crrev.com/121a3d55a69bb82a46ee9e3159a0be1810052368
Cr-Commit-Position: refs/heads/master@{#424748}

[ymalik, 2016-10-06 19:44:37]

ymalik@chromium.org changed reviewers:
+ skobes@chromium.org

[ymalik, 2016-10-06 19:51:14]

https://codereview.chromium.org/2394053004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2394053004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2584: static bool
clearScrollAnchorForScroller(ScrollableArea* scroller,
@skobes, a note to make reviewing easier.

This function can be called in 3 different ways.
FindReferencing: This will walk up the parent chain and return true of
layoutObject is referenced by any other scroller. This is used when we want to
set the isScrollAnchorObject when a scroll anchor is cleared for only a single
scroller (for cases like these:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/Sc...)

ClearReferencing: This will walk up the parent chain and clear the anchors for
scrollers that reference layoutObject. This is needed when an element is removed
from the tree. In this case, we don't want to clear anchors for all scroller in
the parent chain.

ClearAll: This will clear scroll anchors for all parent scrollers of
layoutObject. This is when a non-anchoring scroll happens on a child scroller.

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

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2632: // Continue walk
past frame boundary.
Hmm, do we actually want this behavior?  We don't descend into iframes in
findAnchor, so I don't think iframe scrolls need to clear anchors in parent
frames.

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1663: void
setIsScrollAnchorObject(bool value) {
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.

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1670: void
clearScrollAnchors();
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?

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:38: // containing scroller
and all ancestor scrollers (In certain cases the
You can delete the sentence in parentheses, it's a vestige of bounce
suppression.

[ymalik, 2016-10-11 19:54:23]

@skobes, PTAL :)

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2632: // Continue walk
past frame boundary.
On 2016/10/10 21:34:35, skobes wrote:
> Hmm, do we actually want this behavior?  We don't descend into iframes in
> findAnchor, so I don't think iframe scrolls need to clear anchors in parent
> frames.

hmm you're right. I was under the impression that we do actually descend into
iframes. But we don't so you're right that we don't need this.

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1663: void
setIsScrollAnchorObject(bool value) {
On 2016/10/10 21:34:35, skobes wrote:
> 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.

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).

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1670: void
clearScrollAnchors();
On 2016/10/10 21:34:35, skobes wrote:
> 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?

Done.

[skobes, 2016-10-11 20:51:41]

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1663: void
setIsScrollAnchorObject(bool value) {
On 2016/10/11 19:54:22, ymalik wrote:
> 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.

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.

> 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).

Currently willBeRemovedFromTree clears the bit on itself before calling
findReferencingScrollAnchors, specifically to avoid this problem.

https://codereview.chromium.org/2394053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2394053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:69: void
clearForContainingScroller();
"containing scroller" is a little confusing... maybe call this clearSelf?

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

@skobes, PTAL :)

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2394053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1663: void
setIsScrollAnchorObject(bool value) {
On 2016/10/11 20:51:40, skobes wrote:
> On 2016/10/11 19:54:22, ymalik wrote:
> > 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.
> 
> 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.
> 
> > 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).
> 
> Currently willBeRemovedFromTree clears the bit on itself before calling
> findReferencingScrollAnchors, specifically to avoid this problem.

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.

https://codereview.chromium.org/2394053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2394053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:69: void
clearForContainingScroller();
On 2016/10/11 20:51:41, skobes wrote:
> "containing scroller" is a little confusing... maybe call this clearSelf?

Done.

[skobes, 2016-10-11 23:48:28]

lgtm

[ymalik, 2016-10-12 14:22:29]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-10-12 14:22:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 15:50:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-12 15:52:52]

Description was changed from

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

Before this CL, a call to ScrollAnchor::clear() would clear the anchor node
on the containing scroller, and set the LayoutObject::IsScrollAnchorObject
bit on the anchor if there is no other scroll referencing it.

With this CL, a call to ScrollAnchor::clear() walks up the parent chain and
clears the scroll anchor for every scroller encountered in the walk. As a
result, the LayoutObject::IsScrollAnchorObject is always set to false.

This CL is dependent on https://codereview.chromium.org/2400723002

BUG=650017
==========

to

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

Before this CL, a call to ScrollAnchor::clear() would clear the anchor node
on the containing scroller, and set the LayoutObject::IsScrollAnchorObject
bit on the anchor if there is no other scroll referencing it.

With this CL, a call to ScrollAnchor::clear() walks up the parent chain and
clears the scroll anchor for every scroller encountered in the walk. As a
result, the LayoutObject::IsScrollAnchorObject is always set to false.

This CL is dependent on https://codereview.chromium.org/2400723002

BUG=650017

Committed: https://crrev.com/121a3d55a69bb82a46ee9e3159a0be1810052368
Cr-Commit-Position: refs/heads/master@{#424748}
==========

[commit-bot: I haz the power, 2016-10-12 15:52:53]

Patchset 4 (id:??) landed as
https://crrev.com/121a3d55a69bb82a46ee9e3159a0be1810052368
Cr-Commit-Position: refs/heads/master@{#424748}