Improve scroll anchoring's interactions with the visual viewport

Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876
Committed: https://crrev.com/577f3d408ca3e63ff1a9efc0a95f098df5092fd6
Cr-Commit-Position: refs/heads/master@{#404508}

[ymalik, 2016-07-06 21:27:32]

Description was changed from

==========
Improve scroll anchoring's interactions with the visual viewport

BUG=594876
==========

to

==========
Improve scroll anchoring's interactions with the visual viewport

BUG=594876
==========

[ymalik, 2016-07-06 21:27:32]

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

[ymalik, 2016-07-06 21:29:05]

Description was changed from

==========
Improve scroll anchoring's interactions with the visual viewport

BUG=594876
==========

to

==========
Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876
==========

[skobes, 2016-07-06 23:16:04]

Overall approach looks fine.

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:130: DoubleSize
RootFrameViewport::scrollAmount(DoubleSize delta, const ScrollableArea&
scroller) const
Make this a static helper?

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.h:120: void
setScaleAndLocation(float scale, const FloatPoint& location, bool clearAnchor =
true);
This param is a bit strange, can the caller just clear it?

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:411: LayoutBox*
layoutBox() const override { return &box(); }
Can you add a TODO to get rid of box()?  There isn't much point in having both.

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:112: virtual void
applyAnchoringAdjustment(IntSize adjustment);
Is there any way we can fold this into setScrollPosition (with scrollType ==
AnchoringScroll)?  ScrollableArea already has a confusing plethora of methods to
mutate the scroll offset, I hate to add another.

[ymalik, 2016-07-07 22:01:03]

@skobes PTAL :)

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:130: DoubleSize
RootFrameViewport::scrollAmount(DoubleSize delta, const ScrollableArea&
scroller) const
On 2016/07/06 23:16:04, skobes wrote:
> Make this a static helper?

n/a

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.h:120: void
setScaleAndLocation(float scale, const FloatPoint& location, bool clearAnchor =
true);
On 2016/07/06 23:16:04, skobes wrote:
> This param is a bit strange, can the caller just clear it?

I agree that the param is a bit strange. I think there are too many callers to
clear it. I think we pretty much want to clear the anchor every time the scale
or location is changed and the scroll is not from scroll anchoring. Refactored
the common code.

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:411: LayoutBox*
layoutBox() const override { return &box(); }
On 2016/07/06 23:16:04, skobes wrote:
> Can you add a TODO to get rid of box()?  There isn't much point in having
both.

There's > 200 references to box(). I was originally thinking of calling the
virtual function box(), but that seems vague in general. Do you think it makes
sense to s/box/layoutBox or just have both?

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:112: virtual void
applyAnchoringAdjustment(IntSize adjustment);
On 2016/07/06 23:16:04, skobes wrote:
> Is there any way we can fold this into setScrollPosition (with scrollType ==
> AnchoringScroll)?  ScrollableArea already has a confusing plethora of methods
to
> mutate the scroll offset, I hate to add another.

Yeah you're right. I changed it such that scroll animator calls into
setScrollPosition and setScrollPosition calls the animator's adjust method which
adjusts the animation and sets the scroll position.

[skobes, 2016-07-07 23:31:04]

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:411: LayoutBox*
layoutBox() const override { return &box(); }
On 2016/07/07 22:01:03, ymalik wrote:
> On 2016/07/06 23:16:04, skobes wrote:
> > Can you add a TODO to get rid of box()?  There isn't much point in having
> both.
> 
> There's > 200 references to box(). I was originally thinking of calling the
> virtual function box(), but that seems vague in general. Do you think it makes
> sense to s/box/layoutBox or just have both?

I like s/box/layoutBox/ but in a separate patch (hence being a TODO).

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.h:82: void
distributeScrollBetweenViewports(const DoublePoint&, ScrollType, ScrollBehavior,
bool visualIsPrimary);
Can it be an enum instead of a bool?

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:26: // The scroll that is
scrolled to componsate for layout movements. Note that
"scroll" -> "scroller"

[ymalik, 2016-07-08 14:41:22]

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RootFrameViewport.h:82: void
distributeScrollBetweenViewports(const DoublePoint&, ScrollType, ScrollBehavior,
bool visualIsPrimary);
On 2016/07/07 23:31:03, skobes wrote:
> Can it be an enum instead of a bool?

Done.

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2123053002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:26: // The scroll that is
scrolled to componsate for layout movements. Note that
On 2016/07/07 23:31:03, skobes wrote:
> "scroll" -> "scroller"

Done.

[skobes, 2016-07-08 14:48:05]

lgtm

[ymalik, 2016-07-08 14:51:33]

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

[ymalik, 2016-07-08 14:51:34]

+jbroman for Source/platform

[jbroman, 2016-07-08 17:11:18]

lgtm with a few comments

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

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:26: :
m_scroller(scroller)
super-nit: Minor duplication with the above constructor and the below setter.
WDYT of:

ScrollAnchor::ScrollAnchor(ScrollableArea* scroller)
    : ScrollAnchor()
{
    setScroller(scroller);
}

If you think the logic is likely to diverge, don't bother. Just an idea to avoid
having to update the DCHECKs etc in two places.

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:58: ?
toFrameView(scroller)->layoutViewItem()
Can this function be replaced with:

return LayoutBoxItem(scrollerLayoutBox(scroller));

This avoids duplicating the logic that's embedded elsewhere (in virtual calls).

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:23:
ScrollAnchor(ScrollableArea*);
Not in this CL, but this constructor should probably be explicit to disallow
implicit conversions.

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:275: virtual
LayoutBox* layoutBox() const { return 0; }
nit: 0 -> nullptr

[ymalik, 2016-07-08 17:29:50]

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

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:26: :
m_scroller(scroller)
On 2016/07/08 17:11:17, jbroman wrote:
> super-nit: Minor duplication with the above constructor and the below setter.
> WDYT of:
> 
> ScrollAnchor::ScrollAnchor(ScrollableArea* scroller)
>     : ScrollAnchor()
> {
>     setScroller(scroller);
> }
> 
> If you think the logic is likely to diverge, don't bother. Just an idea to
avoid
> having to update the DCHECKs etc in two places.

Thanks! It's definitely cleaner to do it that way. I don't think the logic will
diverge.

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:58: ?
toFrameView(scroller)->layoutViewItem()
On 2016/07/08 17:11:17, jbroman wrote:
> Can this function be replaced with:
> 
> return LayoutBoxItem(scrollerLayoutBox(scroller));
> 
> This avoids duplicating the logic that's embedded elsewhere (in virtual
calls).

Yeah totally. Done.

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right):

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ScrollAnchor.h:23:
ScrollAnchor(ScrollableArea*);
On 2016/07/08 17:11:18, jbroman wrote:
> Not in this CL, but this constructor should probably be explicit to disallow
> implicit conversions.

Its probably fine to do in this CL. Done.

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2123053002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:275: virtual
LayoutBox* layoutBox() const { return 0; }
On 2016/07/08 17:11:18, jbroman wrote:
> nit: 0 -> nullptr

Done.

[ymalik, 2016-07-08 17:29:56]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-07-08 17:29:57]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2123053002/#ps160001
(title: "jbroman nits")

[commit-bot: I haz the power, 2016-07-08 17:30:31]

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-07-08 19:54:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 19:54:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ymalik, 2016-07-08 20:45:15]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-07-08 20:45:16]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2123053002/#ps180001
(title: "fix broken tests")

[commit-bot: I haz the power, 2016-07-08 20:45:57]

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-07-08 22:18:19]

Description was changed from

==========
Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876
==========

to

==========
Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876
==========

[commit-bot: I haz the power, 2016-07-08 22:18:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-08 22:18:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 22:19:39]

Description was changed from

==========
Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876
==========

to

==========
Improve scroll anchoring's interactions with the visual viewport

ScrollAnchor can now have a RootFrameViewport as its scroller so
that an anchoring adjustment can be distributed between layout
and visual viewports.

BUG=594876

Committed: https://crrev.com/577f3d408ca3e63ff1a9efc0a95f098df5092fd6
Cr-Commit-Position: refs/heads/master@{#404508}
==========

[commit-bot: I haz the power, 2016-07-08 22:19:40]

Patchset 10 (id:??) landed as
https://crrev.com/577f3d408ca3e63ff1a9efc0a95f098df5092fd6
Cr-Commit-Position: refs/heads/master@{#404508}