Use ScrollAnchor in FrameView and PaintLayerScrollableArea.

Use ScrollAnchor in FrameView and PaintLayerScrollableArea.

BUG=558575
Committed: https://crrev.com/f6f66ac71683543ade0945ce9d8951f7b546ec7e
Cr-Commit-Position: refs/heads/master@{#373908}

[skobes, 2016-01-28 02:39:06]

Patchset #1 (id:1) has been deleted

[skobes, 2016-01-28 02:39:17]

Patchset #1 (id:20001) has been deleted

[skobes, 2016-01-29 04:50:39]

Patchset #1 (id:40001) has been deleted

[skobes, 2016-01-29 06:46:27]

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

[ojan, 2016-02-02 06:51:01]

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2021:
scrollToFragmentAnchor();
We'll need to think of what the right behavior is when scrolling to a fragment
anchor scrolling fights with scroll anchoring. I haven't really thought about
it. Mind adding a TODO? Or maybe you've thought this through already.

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:291:
scrollableArea()->scrollAnchor().save();
Can you instead do this in LayoutBlock::layout()? That way it will work for all
of the other LayoutBlock subclasses as well.

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

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2683: layer =
layer->parent();
I think this is O(n^2) in the depth of the layer tree on tear down. We start
teardown at the top of the tree and at each node walk up. I'm a little too
braindead after a day of CSSWG meetings to think about what the more efficient
way to do it is, but I'll mention it for now and CC esprehn who might have good
ideas.

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:416: if
(scrollAnchoringEnabled() && scrollType != AnchoringScroll)
It's a bummer we need to copy-paste this. Maybe put this as a function on
FrameView, then this and the FrameView code could both call something like:
frameView->clearScrollAnchor();

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1460: if
(Frame* frame = box().frame()) {
Nit: If you add a scrollAnchoringEnabled method to Frame, you could probably
drop the versions of that from this class and from FrameView.

[esprehn, 2016-02-02 12:21:46]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-02-02 12:21:46]

Why are we using document settings for scroll anchoring instead of
RuntimeEnabledFeatures?

Document settings are things you can plausibly see doing on a per document
basis, this really doesn't seem like that.

[skobes, 2016-02-02 22:39:11]

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2021:
scrollToFragmentAnchor();
On 2016/02/02 06:51:01, ojan wrote:
> We'll need to think of what the right behavior is when scrolling to a fragment
> anchor scrolling fights with scroll anchoring. I haven't really thought about
> it. Mind adding a TODO? Or maybe you've thought this through already.

Added TODO.  I think the right behavior is for the fragment anchor to override /
suppress the scroll anchor.  I also need to think about history restoration.  We
may want to scroll-anchor only if InitialScrollState::wasScrolledByUser is set.

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:291:
scrollableArea()->scrollAnchor().save();
On 2016/02/02 06:51:01, ojan wrote:
> Can you instead do this in LayoutBlock::layout()? That way it will work for
all
> of the other LayoutBlock subclasses as well.

I thought about this but was scared off by the comment in LayoutBlock::layout:

    // Table cells call layoutBlock directly, so don't add any logic here.  Put
code into
    // layoutBlock().

Do you think it would be better to instrument LayoutBlock::layout and
LayoutTableCell::layout separately?

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

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2683: layer =
layer->parent();
On 2016/02/02 06:51:01, ojan wrote:
> I think this is O(n^2) in the depth of the layer tree on tear down. We start
> teardown at the top of the tree and at each node walk up. I'm a little too
> braindead after a day of CSSWG meetings to think about what the more efficient
> way to do it is, but I'll mention it for now and CC esprehn who might have
good
> ideas.

Yes my first thought was to add a back pointer to the scroller in LayoutObject,
but it seems bad for every LayoutObject to have this.  It would be a natural fit
in a LayoutObjectRareData struct, which doesn't currently exist.  Or we can add
a hashmap.  Though there was a thread on blink-dev a while back about hashmaps
being worse than data members.  So I decided to do the simple thing for now. :)

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:416: if
(scrollAnchoringEnabled() && scrollType != AnchoringScroll)
On 2016/02/02 06:51:01, ojan wrote:
> It's a bummer we need to copy-paste this. Maybe put this as a function on
> FrameView, then this and the FrameView code could both call something like:
> frameView->clearScrollAnchor();

Yes, this is why we want root layer scrolling. :)

It wouldn't really make sense to call frameView->clearScrollAnchor() here, since
we want to clear our own scroll anchor and not the FrameView's.

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1460: if
(Frame* frame = box().frame()) {
On 2016/02/02 06:51:01, ojan wrote:
> Nit: If you add a scrollAnchoringEnabled method to Frame, you could probably
> drop the versions of that from this class and from FrameView.

Hmm, this doesn't save us very much, and it increases the number of classes that
need to know about scroll anchoring.

In general I wish we would change the way settings are plumbed to make it easier
to access.  There are too many indirections and ad hoc null checks.

[skobes, 2016-02-02 22:39:50]

On 2016/02/02 12:21:46, esprehn wrote:
> Why are we using document settings for scroll anchoring instead of
> RuntimeEnabledFeatures?
> 
> Document settings are things you can plausibly see doing on a per document
> basis, this really doesn't seem like that.

Settings is owned by Page, it's not per document.

[esprehn, 2016-02-02 23:24:09]

On 2016/02/02 at 22:39:50, skobes wrote:
> On 2016/02/02 12:21:46, esprehn wrote:
> > Why are we using document settings for scroll anchoring instead of
> > RuntimeEnabledFeatures?
> > 
> > Document settings are things you can plausibly see doing on a per document
> > basis, this really doesn't seem like that.
> 
> Settings is owned by Page, it's not per document.

That doesn't really answer the question, why isn't this a runtime feature? Will
two tabs in the same process have different values?

[skobes, 2016-02-02 23:33:55]

On 2016/02/02 23:24:09, esprehn wrote:
> On 2016/02/02 at 22:39:50, skobes wrote:
> > On 2016/02/02 12:21:46, esprehn wrote:
> > > Why are we using document settings for scroll anchoring instead of
> > > RuntimeEnabledFeatures?
> > > 
> > > Document settings are things you can plausibly see doing on a per document
> > > basis, this really doesn't seem like that.
> > 
> > Settings is owned by Page, it's not per document.
> 
> That doesn't really answer the question, why isn't this a runtime feature?
Will
> two tabs in the same process have different values?

No, it's hooked up to a flag (--enable-scroll-anchoring).

I have no objection to it being in RuntimeEnabledFeatures.  Though there are
many "global" settings so the distinction seems a bit academic.

[ojan, 2016-02-03 01:23:44]

RuntimeEnabledFeatures is exactly the intended solution for the wish you
expressed earlier that we have a way to do settings that doesn't have so much
plumbing and ad-hoc null-checking. It has a number of advantages over Settings:
1. Let's you easily control which builds it's enabled in (test, experimental,
stable) in a way that's standard and easy for people to quickly lookup.
2. It removes all the null-checks needed to get a Settings.
3. It's done via static functions, so doesn't need all the helper functions
since you can just use the static function inline.

TL;DR: We should move anything from Settings that isn't required to be
per-document/page to RuntimeEnabledFeatures. That's a half-finished project that
noone is working on.

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:2021:
scrollToFragmentAnchor();
On 2016/02/02 at 22:39:10, skobes wrote:
> On 2016/02/02 06:51:01, ojan wrote:
> > We'll need to think of what the right behavior is when scrolling to a
fragment
> > anchor scrolling fights with scroll anchoring. I haven't really thought
about
> > it. Mind adding a TODO? Or maybe you've thought this through already.
> 
> Added TODO.  I think the right behavior is for the fragment anchor to override
/ suppress the scroll anchor.

I agree.

>  I also need to think about history restoration.  We may want to scroll-anchor
only if InitialScrollState::wasScrolledByUser is set.

I'm less sure of that. If history or fragment anchor causes scroll and then an
image loads above you, it would still be jarring to have the content shift
around. So, at first glance, I think we want to scroll anchor in all cases and
to treat fragment/history scrolling the same way we would a user scroll.

https://codereview.chromium.org/1647793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1647793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2686: // TODO(skobes):
Should this be a hash lookup or a data member instead?
I was hoping this TODO would explicitly point out that this is n^2.

Now that I'm a bit more awake, I can think of a potential solution. I agree that
a hash table or data member is bad. I think we need a bit on LayoutObject as to
whether this object is a scroll anchor. That way we only do the ancestor walk
for things that actually are scroll anchors, which removes the n^2. That's easy
enough to do that maybe we don't even need the TODO and you can roll it into
this patch?

[pdr., 2016-02-03 03:33:49]

On 2016/02/03 at 01:23:44, ojan wrote:
> TL;DR: We should move anything from Settings that isn't required to be
per-document/page to RuntimeEnabledFeatures. That's a half-finished project that
noone is working on.

I had quite the fiesta in SVGLand trying to plumb global font settings.
RuntimeEnabledFeatures are good for some global settings flags, but we need a
good home for real global settings too.

[ojan, 2016-02-03 04:56:42]

On 2016/02/03 at 03:33:49, pdr wrote:
> On 2016/02/03 at 01:23:44, ojan wrote:
> > TL;DR: We should move anything from Settings that isn't required to be
per-document/page to RuntimeEnabledFeatures. That's a half-finished project that
noone is working on.
> 
> I had quite the fiesta in SVGLand trying to plumb global font settings.
RuntimeEnabledFeatures are good for some global settings flags, but we need a
good home for real global settings too.

Actually, it looks like the vast majority of things in Settings.in are actually
global. We should probably change Settings.in to have global by default and you
can opt individual things into being per-page. But, that's a project for another
day. In this case, this is an experimental feature, so fits well into the
RuntimeEnabledFeature model. It'd be great if someone tried to merge the two
into one concept actually.

[ojan, 2016-02-03 06:17:52]

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1647793002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:291:
scrollableArea()->scrollAnchor().save();
On 2016/02/02 at 22:39:10, skobes wrote:
> On 2016/02/02 06:51:01, ojan wrote:
> > Can you instead do this in LayoutBlock::layout()? That way it will work for
all
> > of the other LayoutBlock subclasses as well.
> 
> I thought about this but was scared off by the comment in LayoutBlock::layout:
> 
>     // Table cells call layoutBlock directly, so don't add any logic here. 
Put code into
>     // layoutBlock().
> 
> Do you think it would be better to instrument LayoutBlock::layout and
LayoutTableCell::layout separately?

Yes, that way the code will work for flex/grid/etc as well. At some point we
should make test cases for all the different display types, but I don't think
that needs to block this patch landing. Are you keeping track somewhere for
test-cases we should write? Each case we find that didn't do the right thing, we
should make a test case for so that at the end we have a nice robust test suite
to use to build a spec and to share with other browser vendors.

[skobes, 2016-02-03 06:17:55]

On 2016/02/03 01:23:44, ojan wrote:
> RuntimeEnabledFeatures is exactly the intended solution for the wish you
> expressed earlier that we have a way to do settings that doesn't have so much
> plumbing and ad-hoc null-checking. It has a number of advantages over
Settings:
> 1. Let's you easily control which builds it's enabled in (test, experimental,
> stable) in a way that's standard and easy for people to quickly lookup.
> 2. It removes all the null-checks needed to get a Settings.
> 3. It's done via static functions, so doesn't need all the helper functions
> since you can just use the static function inline.

Ok, I am convinced.  http://crrev.com/1662933003 moves scroll anchoring to
RuntimeEnabledFeatures.

> Now that I'm a bit more awake, I can think of a potential solution. I agree
that
> a hash table or data member is bad. I think we need a bit on LayoutObject as
to
> whether this object is a scroll anchor. That way we only do the ancestor walk
> for things that actually are scroll anchors, which removes the n^2. That's
easy
> enough to do that maybe we don't even need the TODO and you can roll it into
> this patch?

Thanks, I like this idea.

Unfortunately the bit on LayoutObject is exposing some lifetime issues.  It
looks like LayoutObjectChildList::removeChildNode won't call
willBeRemovedFromTree if the document is being destroyed, so ScrollAnchor holds
onto a stale LayoutObject* (and crashes trying to reset the bit from its dtor).

I'll continue investigating tomorrow (but let me know if you have any
suggestions).

[skobes, 2016-02-04 01:47:45]

PTAL.

The lifetime issue was resolved by calling clear() from the scroller's dispose()
method instead of trying to do anything in ~ScrollAnchor.

I think we don't actually need to instrument LayoutTableCell because as far as I
can tell <td> elements cannot have vertical overflow.

I'll make a list of test cases and add it to the main scroll anchoring doc.

[ojan, 2016-02-05 11:01:52]

lgtm

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:45: m_savedRelativeOffset
= computeRelativeOffset(m_scroller, m_anchorObject);
I think this could be inside the if clause above as well? Any time something
changes that could affect this offset, we have to clear the anchorObject
anyways. So, with that, you could just have an if (m_anchorObject) early return.

[skobes, 2016-02-05 17:52:32]

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:45: m_savedRelativeOffset
= computeRelativeOffset(m_scroller, m_anchorObject);
On 2016/02/05 11:01:52, ojan wrote:
> I think this could be inside the if clause above as well? Any time something
> changes that could affect this offset, we have to clear the anchorObject
> anyways. So, with that, you could just have an if (m_anchorObject) early
return.

In theory, yes, but that seems brittle to me.  Suppose something changes that
affects this offset, but we fail to clear the anchor.  If save() always
recalculates the relative offset, we will just anchor to a poorly-chosen node. 
But if we cache the offset we may *create* a visible jump where there wouldn't
have been one, which is much worse.

[skobes, 2016-02-05 17:52:37]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-02-05 17:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1647793002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1647793002/160001

[commit-bot: I haz the power, 2016-02-05 21:01:33]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-02-05 21:04:53]

Description was changed from

==========
Use ScrollAnchor in FrameView and PaintLayerScrollableArea.

BUG=558575
==========

to

==========
Use ScrollAnchor in FrameView and PaintLayerScrollableArea.

BUG=558575

Committed: https://crrev.com/f6f66ac71683543ade0945ce9d8951f7b546ec7e
Cr-Commit-Position: refs/heads/master@{#373908}
==========

[commit-bot: I haz the power, 2016-02-05 21:04:54]

Patchset 6 (id:??) landed as
https://crrev.com/f6f66ac71683543ade0945ce9d8951f7b546ec7e
Cr-Commit-Position: refs/heads/master@{#373908}

[ojan, 2016-02-06 00:09:27]

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/1647793002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:45: m_savedRelativeOffset
= computeRelativeOffset(m_scroller, m_anchorObject);
On 2016/02/05 at 17:52:31, skobes wrote:
> On 2016/02/05 11:01:52, ojan wrote:
> > I think this could be inside the if clause above as well? Any time something
> > changes that could affect this offset, we have to clear the anchorObject
> > anyways. So, with that, you could just have an if (m_anchorObject) early
return.
> 
> In theory, yes, but that seems brittle to me.  Suppose something changes that
affects this offset, but we fail to clear the anchor.  If save() always
recalculates the relative offset, we will just anchor to a poorly-chosen node. 
But if we cache the offset we may *create* a visible jump where there wouldn't
have been one, which is much worse.

I can't think of any scenario in which it would make sense to keep the anchor
but not the offset. If we had such a thing, it would represent a real bug that
we need to fix anyways, so this extra defensiveness would just mask the bug.
computeRelativeOffset will be doing real work on each layout. It's not the end
of the world, but it's one of a thousand cuts that isn't serving a purpose.

[skobes, 2016-02-06 01:00:53]

On 2016/02/06 00:09:27, ojan wrote:
> I can't think of any scenario in which it would make sense to keep the anchor
> but not the offset. If we had such a thing, it would represent a real bug that
> we need to fix anyways, so this extra defensiveness would just mask the bug.
> computeRelativeOffset will be doing real work on each layout. It's not the end
> of the world, but it's one of a thousand cuts that isn't serving a purpose.

Alright, I have adopted your suggestion in http://crrev.com/1640973005.