Fix scroll anchoring during viewport resize.

Fix scroll anchoring during viewport resize.

WebView::resizeWithTopControls does a synchronous layout.  Anchoring scrolls
performed during this layout were getting clobbered by ResizeViewportAnchor,
whose purpose is to scroll the viewports to preserve their combined position
(as reported by RootFrameViewport) after bounds-clamping to their new sizes.

After this change, ResizeViewportAnchor tracks the resize-induced scroll delta
while ignoring other scrolls.

BUG=626258, 612305
Committed: https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70
Cr-Commit-Position: refs/heads/master@{#408439}

[skobes, 2016-07-28 02:13:34]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 02:13:53]

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

[skobes, 2016-07-28 03:16:10]

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

[commit-bot: I haz the power, 2016-07-28 03:33:53]

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

[commit-bot: I haz the power, 2016-07-28 03:33:54]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-07-28 14:55:55]

lgtm % question and request for comment.

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:25:
frameView->resize(size);
I was a little concerned about this at first since the resize call here can
cause both a ScrollAnchor and a clamp, one of which this anchor wants to adjust
for, but after thinking about it some more I think we may be ok.

The reason we wanted to anchor the clamp prior to this was that we set the
topControlsAdjustment before the frame resize which means our scroll position
goes out of bounds and we clamp because of the didUpdateTopControls anchor. But
because the didUpdateTopControls anchor is now a no-op in the
WebViewImpl::resizeWithTopControls case, there should be no clamp between
setting the topControlsAdjustment and  resizing the FrameView. Since the
FrameView can't scroll out of bounds then we should never actually clamp it
here.

Is this your understanding?

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ResizeViewportAnchor.h (right):

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ResizeViewportAnchor.h:58: DoubleSize m_drift;
Could you add a comment describing this member? I'm assuming it's the amount
scroll was adjusted by ScrollAnchoring/layout?

[skobes, 2016-07-28 15:45:50]

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:25:
frameView->resize(size);
On 2016/07/28 14:55:55, bokan wrote:
> I was a little concerned about this at first since the resize call here can
> cause both a ScrollAnchor and a clamp, one of which this anchor wants to
adjust
> for, but after thinking about it some more I think we may be ok.

ScrollAnchor shouldn't actually be doing anything during FrameView::resize,
since there's no layout happening (though this is a little hard to see since we
call restore from updateScrollbars).

We want ResizeViewportAnchor to track drift from the resize, but not from
ScrollAnchor.

> The reason we wanted to anchor the clamp prior to this was that we set the
> topControlsAdjustment before the frame resize which means our scroll position
> goes out of bounds and we clamp because of the didUpdateTopControls anchor.
But
> because the didUpdateTopControls anchor is now a no-op in the
> WebViewImpl::resizeWithTopControls case, there should be no clamp between
> setting the topControlsAdjustment and  resizing the FrameView. Since the
> FrameView can't scroll out of bounds then we should never actually clamp it
> here.

I think this is right.  I made nested RVA a nop because otherwise the outer RVA
needs to track drift from the inner RVA, which seemed a bit silly.

But the nice thing about this design is that in principle the clamps can happen
at any time as long as we plumb them through RVA.  For example if we want to
clamp after setting the top controls adjustment we can add
RVA::setTopControlsAdjustment which updates m_drift in the same way as
RVA::resizeFrameView.

In this change I'm being as conservative as possible but I think this gives us a
good foundation to move toward a world where scroll positions are always valid.

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ResizeViewportAnchor.h (right):

https://codereview.chromium.org/2184333002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ResizeViewportAnchor.h:58: DoubleSize m_drift;
On 2016/07/28 14:55:55, bokan wrote:
> Could you add a comment describing this member? I'm assuming it's the amount
> scroll was adjusted by ScrollAnchoring/layout?

The opposite actually :) Comment added.

[skobes, 2016-07-28 15:46:19]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 15:46:32]

Dry run: 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-28 17:19:47]

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

[commit-bot: I haz the power, 2016-07-28 17:19:48]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-07-28 18:23:48]

Ok, thanks for the explanation. I got a little confused in endScope() but I get
it now. still lgtm

[skobes, 2016-07-28 18:31:58]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2016-07-28 18:32:29]

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-28 18:37:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-28 18:40:23]

Description was changed from

==========
Fix scroll anchoring during viewport resize.

WebView::resizeWithTopControls does a synchronous layout.  Anchoring scrolls
performed during this layout were getting clobbered by ResizeViewportAnchor,
whose purpose is to scroll the viewports to preserve their combined position
(as reported by RootFrameViewport) after bounds-clamping to their new sizes.

After this change, ResizeViewportAnchor tracks the resize-induced scroll delta
while ignoring other scrolls.

BUG=626258, 612305
==========

to

==========
Fix scroll anchoring during viewport resize.

WebView::resizeWithTopControls does a synchronous layout.  Anchoring scrolls
performed during this layout were getting clobbered by ResizeViewportAnchor,
whose purpose is to scroll the viewports to preserve their combined position
(as reported by RootFrameViewport) after bounds-clamping to their new sizes.

After this change, ResizeViewportAnchor tracks the resize-induced scroll delta
while ignoring other scrolls.

BUG=626258, 612305

Committed: https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70
Cr-Commit-Position: refs/heads/master@{#408439}
==========

[commit-bot: I haz the power, 2016-07-28 18:40:25]

Patchset 2 (id:??) landed as
https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70
Cr-Commit-Position: refs/heads/master@{#408439}