Make scroll anchoring adjustment when the bounds of a scroller is changed

Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

[ymalik, 2016-07-14 14:36:23]

Description was changed from

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

to

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

[ymalik, 2016-07-14 14:36:23]

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

[skobes, 2016-07-14 14:49:25]

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

https://codereview.chromium.org/2152773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3652: // that is outside its
new bounds. Calling ScrollAnchor::restore
If the scroll bounds have changed, we will relayout.  Why wouldn't we have
already restored by this point?

[skobes, 2016-07-14 15:29:08]

lgtm per offline discussion, though it's somewhat unfortunate to call restore()
in two different places.

Also please verify that we don't need anything similar in LayoutBlock / PLSA.

[ymalik, 2016-07-15 16:08:54]

@skobes, PTAL

I find this to be cleaner than duplicating code. I am not a fan of exposing
restore to scrollable area but its probably okay?

[skobes, 2016-07-15 17:08:24]

On 2016/07/15 16:08:54, ymalik wrote:
> @skobes, PTAL
> 
> I find this to be cleaner than duplicating code. I am not a fan of exposing
> restore to scrollable area but its probably okay?

It's surprising for clampScrollPosition() to do the anchoring, since clamping
and anchoring are separate concepts.  Can FV/PLSA call restore() before
clamping?

[ymalik, 2016-07-15 20:10:57]

PTAL :)

Its probably better to call restore/save from PLSA/FV only when needed (bounds
actually change). We don't get much value from having clampScrollPosition
refactored. Added a TODO to remove the duplicate logic in FV.

[skobes, 2016-07-15 20:17:42]

lgtm w/ nit

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

https://codereview.chromium.org/2152773002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3653: // ensures that no
visible jump is seen when clamping.
I think this comment should mention that we restore before clamping because
clamping clears the scroll anchor.

[ymalik, 2016-07-18 16:08:48]

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

https://codereview.chromium.org/2152773002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3653: // ensures that no
visible jump is seen when clamping.
On 2016/07/15 20:17:42, skobes wrote:
> I think this comment should mention that we restore before clamping because
> clamping clears the scroll anchor.

Done.

[ymalik, 2016-07-18 16:08:55]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-07-18 16:08:56]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2152773002/#ps100001
(title: "update comment + rebase")

[commit-bot: I haz the power, 2016-07-18 16:09:16]

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-18 17:25:35]

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

[commit-bot: I haz the power, 2016-07-18 17:25:37]

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

[ymalik, 2016-07-19 13:49:36]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-07-19 13:49:36]

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

[commit-bot: I haz the power, 2016-07-19 13:50:01]

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-19 15:19:39]

Description was changed from

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

to

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

[commit-bot: I haz the power, 2016-07-19 15:19:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-19 15:20:53]

Description was changed from

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

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

to

==========
Make scroll anchoring adjustment when the bounds of a scroller is changed

A layout update can change the bounds of a scroller, requiring us to clamp the
current scroll position if its outside the bounds of the scroller. This would
clear the saved scroll anchor and have a visible jump. The main issue is that
we would clear the anchor between ScrollAnchor::save and ScrollAnchor::restore.

This CL calls ::restore if clamping is needed to ensure that scroll anchoring
still works.

BUG=594876

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

[commit-bot: I haz the power, 2016-07-19 15:20:54]

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