Adjust maximum scroll bounds on FrameView to account for top controls. (Blink-side)

Adjust maximum scroll bounds on FrameView to account for top controls. (Blink-side)

The compositor adjusts the viewport bounds based on how far the top
controls are shown; however, the resize of the viewport on the Blink
side doesn't happen until a Resize event is received, which is heavily
throttled. This means that when a commit happens before the Resize,
the scroll offsets given to Blink may be outside the maximum bounds
and will be incorrectly clamped.

This CL adjusts FrameView's maximumScrollPosition to account for the
drift in where Blink and the Compositor think the top controls are.

Chromium-side: https://codereview.chromium.org/563513002

BUG=412581
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182600

[bokan, 2014-09-09 23:07:28]

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

[bokan, 2014-09-09 23:09:15]

PTAL

[bokan, 2014-09-09 23:14:20]

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1689:
view->setTopControlsViewportAdjustment(topControlsViewportAdjustment);
There's also a Chromium-side patch that just calls setTopControlsLayoutHeight in
RenderWidget::Resize. I'll land this patch first with this line commented out so
it doesn't change behaviour.

[aelias_OOO_until_Jul13, 2014-09-10 01:57:29]

lgtm modulo minor comments

https://codereview.chromium.org/560623002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/560623002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:498: int m_topControlsViewportAdjustment;
Please make this a float.

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1689:
view->setTopControlsViewportAdjustment(topControlsViewportAdjustment);
On 2014/09/09 at 23:14:19, bokan wrote:
> There's also a Chromium-side patch that just calls setTopControlsLayoutHeight
in RenderWidget::Resize. I'll land this patch first with this line commented out
so it doesn't change behaviour.

If you prefer, you could also avoid the third patch by making this conditional
on pinch viewport mode being active, as nobody cares about temporary breakage
there yet.

[bokan, 2014-09-10 13:51:52]

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

[bokan, 2014-09-10 13:51:53]

+eseidel@ for Source/core and public/web OWNER

https://codereview.chromium.org/560623002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/560623002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:498: int m_topControlsViewportAdjustment;
On 2014/09/10 01:57:29, aelias wrote:
> Please make this a float.

Done.

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1689:
view->setTopControlsViewportAdjustment(topControlsViewportAdjustment);
On 2014/09/10 01:57:29, aelias wrote:
> On 2014/09/09 at 23:14:19, bokan wrote:
> > There's also a Chromium-side patch that just calls
setTopControlsLayoutHeight
> in RenderWidget::Resize. I'll land this patch first with this line commented
out
> so it doesn't change behaviour.
> 
> If you prefer, you could also avoid the third patch by making this conditional
> on pinch viewport mode being active, as nobody cares about temporary breakage
> there yet.

One liners like that are easy enough to land + I'd have to change up the test.
Side-note: can I just TBR-commit the patch to uncomment?

[aelias_OOO_until_Jul13, 2014-09-10 15:51:46]

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/560623002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1689:
view->setTopControlsViewportAdjustment(topControlsViewportAdjustment);
On 2014/09/10 13:51:52, bokan wrote:
> On 2014/09/10 01:57:29, aelias wrote:
> > On 2014/09/09 at 23:14:19, bokan wrote:
> > > There's also a Chromium-side patch that just calls
> setTopControlsLayoutHeight
> > in RenderWidget::Resize. I'll land this patch first with this line commented
> out
> > so it doesn't change behaviour.
> > 
> > If you prefer, you could also avoid the third patch by making this
conditional
> > on pinch viewport mode being active, as nobody cares about temporary
breakage
> > there yet.
> 
> One liners like that are easy enough to land + I'd have to change up the test.
> Side-note: can I just TBR-commit the patch to uncomment?

Yes, feel free to TBR that and mention I preapproved that here.

[leviw_travelin_and_unemployed, 2014-09-10 20:15:50]

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

[leviw_travelin_and_unemployed, 2014-09-10 20:15:51]

When are we setting the topControlsLayoutHeight on WebViewImpl?

https://codereview.chromium.org/560623002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/560623002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2994: max.move(0,
m_topControlsViewportAdjustment / visibleContentScaleFactor());
Don't you want to ceil this value? If so, I'd expect a test that ensures we
don't lose a pixel.

[bokan, 2014-09-15 20:30:19]

topControlsLayoutHeight gets set in RenderWidget::Resize (i.e. when we get a
WebView resize). That's added in the chromium-side patch linked in the
description.

https://codereview.chromium.org/560623002/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/560623002/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:2994: max.move(0,
m_topControlsViewportAdjustment / visibleContentScaleFactor());
On 2014/09/10 20:15:51, leviw wrote:
> Don't you want to ceil this value? If so, I'd expect a test that ensures we
> don't lose a pixel.

I think we want to floor in this case (since we ceil the visibleSize, we should
floor anything we take away from it), but I'll admit this isn't terribly clear.
I've rewritten it to essentially the following:

max = contentHeight - ceil((visibleHeight-adjustment)/scale)

Which is what happens on the CC side. Also added to the test to check the
fractional pixel case.

[bokan, 2014-09-16 21:41:08]

bokan@chromium.org changed reviewers:
- eseidel@chromium.org

[bokan, 2014-09-16 21:41:20]

Friendly ping.

[bokan, 2014-09-23 00:28:34]

On 2014/09/16 21:41:20, bokan wrote:
> Friendly ping.

Hey Levi, PTAL.

[bokan, 2014-09-23 15:47:09]

On 2014/09/23 00:28:34, bokan wrote:
> On 2014/09/16 21:41:20, bokan wrote:
> > Friendly ping.
> 
> Hey Levi, PTAL.

Doh! Just realized the updated patch set was never uploaded...it's up now, sorry
about noise.

[leviw_travelin_and_unemployed, 2014-09-23 18:56:59]

LGTM.

[bokan, 2014-09-23 19:08:14]

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

[bokan, 2014-09-23 19:08:15]

+jamesr@ again for public/web

[jamesr, 2014-09-24 04:42:01]

weird, but lgtm

[bokan, 2014-09-24 13:25:21]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-09-24 13:26:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560623002/60001

[commit-bot: I haz the power, 2014-09-24 14:33:03]

Committed patchset #4 (id:60001) as 182600