Fix main thread top controls scrolling to mirror CC.

Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938
Cr-Commit-Position: refs/heads/master@{#390085}

[bokan, 2016-03-30 17:49:35]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too.

BUG=597266
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-03-30 17:52:20]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-03-30 17:52:21]

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

[bokan, 2016-03-30 17:53:06]

PTAL, thanks!

[bokan, 2016-04-01 13:57:45]

friendly ping :)

[majidvp, 2016-04-01 17:09:36]

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

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:477: // FIXME: We
probably shouldn't be storing the bounds in a float. crbug.com/422331.
The actual bug for this TODO is marked as WontFix. Does this need to be updated?

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:489:
viewportSize.expand(0, ceilf(m_topControlsAdjustment));
Can you point out where in CC we are ceiling the top controls adjustment?

It is a bit odd that we ceil the top controls adjustment when computing 
viewportSize and floor it when computing frameViewSize. Ideally we 
only clamp it at the end of the computation. For example with a 
scale of 1 the current behaviour can adjust the max scroll offset
by one pixel though it was not necessary. 

Should we instead floor maxPosition instead?

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:1849: // changed. We'll try to
restore hte saved scroll offset when anchor
s/hte/the/

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:1852:
view->setScrollPosition(view->scrollPositionDouble(), ProgrammaticScroll);
Shouldn't this be unnecessary? The anchor sets the scroll position which should
also
clamp if necessary.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:777:
EXPECT_EQ(expectedVisualOffset + expectedLayoutOffset,
rootViewport->scrollPosition().y());
s/expectedVisualOffset + expectedLayoutOffset/expectedRootOffset/

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:783:
webView->resize(WebSize(800, 600));
600? did you mean layoutViewportHeight + topControlsHeight

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:788: int
previousRootOffset = expectedVisualOffset + expectedLayoutOffset;
You can use expectedRootOffset which should be defined before :)

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:802:
webView->handleInputEvent(generateEvent(WebInputEvent::GestureScrollUpdate, 0,
100));
100 is the same as topControlsHeight which may be confusing.
The logic you are testing should work for any amount of 
top control movement.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:805:
view->setScrollPosition(view->scrollPosition(), ProgrammaticScroll);
Why are these clamps required?

[bokan, 2016-04-01 20:36:01]

Patchset #4 (id:60001) has been deleted

[bokan, 2016-04-04 21:16:06]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=597266
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-04-04 22:36:10]

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

[commit-bot: I haz the power, 2016-04-04 22:36:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/120001

[commit-bot: I haz the power, 2016-04-04 23:05:27]

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

[commit-bot: I haz the power, 2016-04-04 23:05:28]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[bokan, 2016-04-05 00:11:56]

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

[commit-bot: I haz the power, 2016-04-05 00:12:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/150001

[commit-bot: I haz the power, 2016-04-05 00:19:12]

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

[commit-bot: I haz the power, 2016-04-05 00:19:13]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[bokan, 2016-04-05 01:48:55]

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

[commit-bot: I haz the power, 2016-04-05 01:49:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/170001

[commit-bot: I haz the power, 2016-04-05 02:15:47]

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

[commit-bot: I haz the power, 2016-04-05 02:15:48]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[bokan, 2016-04-05 15:36:55]

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

[commit-bot: I haz the power, 2016-04-05 15:37:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/210001

[commit-bot: I haz the power, 2016-04-05 16:49:53]

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

[commit-bot: I haz the power, 2016-04-05 16:49:54]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-04-05 17:11:41]

I had to expand the scope a bit since I found a problem with the original patch
I sent you. Because we now clamp the scroll offsets and anchor when top controls
change, we can't, in general, change the top controls height and resize the
Blink size separately. I've merged the two into the WebView::resize (and
WebWidget) call. Feel free to review just the main thread bits and I'll get
aelias@ to approve the rest.

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

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:477: // FIXME: We
probably shouldn't be storing the bounds in a float. crbug.com/422331.
On 2016/04/01 17:09:36, majidvp wrote:
> The actual bug for this TODO is marked as WontFix. Does this need to be
updated?

Good catch, this was the wrong bug. Updated.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:489:
viewportSize.expand(0, ceilf(m_topControlsAdjustment));
On 2016/04/01 17:09:36, majidvp wrote:
> Can you point out where in CC we are ceiling the top controls adjustment?
> 
> It is a bit odd that we ceil the top controls adjustment when computing 
> viewportSize and floor it when computing frameViewSize. Ideally we 
> only clamp it at the end of the computation. For example with a 
> scale of 1 the current behaviour can adjust the max scroll offset
> by one pixel though it was not necessary. 
> 
> Should we instead floor maxPosition instead?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/property_...

We ceil the bounds after adding the bounds_delta (bounds_delta is basically the
CC equivalent of topControlsAdjustment) when computing the clip size. I agree
it's odd that we're doing this but I'd rather match CC's quirks here as the CC
path is much better tested. We can adjust both sides in a follow up, though I'm
also weary of changing this code that isn't showing any obvious bugs.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:1849: // changed. We'll try to
restore hte saved scroll offset when anchor
On 2016/04/01 17:09:36, majidvp wrote:
> s/hte/the/

Done.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:1852:
view->setScrollPosition(view->scrollPositionDouble(), ProgrammaticScroll);
On 2016/04/01 17:09:36, majidvp wrote:
> Shouldn't this be unnecessary? The anchor sets the scroll position which
should
> also
> clamp if necessary.

But the anchor needs the viewports to be at their clamped locations in order to
calculate where to restore them to (it works based on deltas in order to prevent
scrolling the layout viewport if it's unneeded). Admittedly this is more
appropriate in the anchoring code so I moved it there.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/TopControlsTest.cpp (right):

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:777:
EXPECT_EQ(expectedVisualOffset + expectedLayoutOffset,
rootViewport->scrollPosition().y());
On 2016/04/01 17:09:36, majidvp wrote:
> s/expectedVisualOffset + expectedLayoutOffset/expectedRootOffset/

Done.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:783:
webView->resize(WebSize(800, 600));
On 2016/04/01 17:09:36, majidvp wrote:
> 600? did you mean layoutViewportHeight + topControlsHeight

Done.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:788: int
previousRootOffset = expectedVisualOffset + expectedLayoutOffset;
On 2016/04/01 17:09:36, majidvp wrote:
> You can use expectedRootOffset which should be defined before :)

Done.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:802:
webView->handleInputEvent(generateEvent(WebInputEvent::GestureScrollUpdate, 0,
100));
On 2016/04/01 17:09:36, majidvp wrote:
> 100 is the same as topControlsHeight which may be confusing.
> The logic you are testing should work for any amount of 
> top control movement.

Changed to 80.

https://codereview.chromium.org/1844013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/TopControlsTest.cpp:805:
view->setScrollPosition(view->scrollPosition(), ProgrammaticScroll);
On 2016/04/01 17:09:36, majidvp wrote:
> Why are these clamps required?

If we don't run the anchoring logic, the viewport scroll offsets will simply
remain at the invalid scroll positions so the check below would pass. This
forces the scroll offsets to valid values.

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1326:
TEST_F(VisualViewportTest, TestTopControlsAdjustmentAndResize)
I think this test was accidentally testing something that generally doesn't
happen, scrolling to the bottom of the page but showing the top controls. I've
fixed it to be more realistic and clear (scrolling down generally hides the top
controls). It can technically happen though, suppose you scroll up, top controls
show up but the page does a scrollTo(infinity) before the user lifts their
finger (bokan.ca/clamp.html). I've added a test case for that too below.

[majidvp, 2016-04-06 18:07:30]

https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
content/renderer/render_widget.cc:1151: top_controls_shrink_blink_size_);
I think for the top control anchoring to work correctly
you also need to resize the visual viewport in the same call.
This mean folding WebWidget::resizeVisualViewport into
the same same resize call.

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:52: IntSize layoutDelta =
delta.height() < 0
nit: Can we just round here? There does not seem to be any
advantage of going to the next pixel as opposed to the closest pixel.

[bokan, 2016-04-06 19:23:35]

https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
content/renderer/render_widget.cc:1151: top_controls_shrink_blink_size_);
On 2016/04/06 18:07:30, majidvp wrote:
> I think for the top control anchoring to work correctly
> you also need to resize the visual viewport in the same call.
> This mean folding WebWidget::resizeVisualViewport into
> the same same resize call.

We already do this in WebViewImpl::performResize. When resize is called, both
viewports are resized. The call below to resizeVisualViewport is for the OSK
where we might not have a layout affecting resize (i.e. resized == false above).

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):

https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:52: IntSize layoutDelta =
delta.height() < 0
On 2016/04/06 18:07:30, majidvp wrote:
> nit: Can we just round here? There does not seem to be any
> advantage of going to the next pixel as opposed to the closest pixel. 

I don't think we can. We scroll the visual viewport first. Once that's maximally
scrolled, we scroll the remainder in the FrameView and then fixup the subpixel
part in the visual viewport since FrameView doesn't support fractionals. Assume
our delta is positive. If we round down the FrameView offset, the VisualViewport
is maximally scrolled so it won't be able to compensate for the fractional since
it's bounded by the FrameView. We need to "overscroll" the FrameView to ensure
the pixel of which we want a fraction of is within the FrameView. Does that make
sense?

[majidvp, 2016-04-06 21:23:16]

On 2016/04/06 at 19:23:35, bokan wrote:
>
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> content/renderer/render_widget.cc:1151: top_controls_shrink_blink_size_);
> On 2016/04/06 18:07:30, majidvp wrote:
> > I think for the top control anchoring to work correctly
> > you also need to resize the visual viewport in the same call.
> > This mean folding WebWidget::resizeVisualViewport into
> > the same same resize call.
> 
> We already do this in WebViewImpl::performResize. When resize is called, both
viewports are resized. The call below to resizeVisualViewport is for the OSK
where we might not have a layout affecting resize (i.e. resized == false above).
> 

Got it. Thanks for the info.

>
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):
> 
>
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:52: IntSize layoutDelta
= delta.height() < 0
> On 2016/04/06 18:07:30, majidvp wrote:
> > nit: Can we just round here? There does not seem to be any
> > advantage of going to the next pixel as opposed to the closest pixel. 
> 
> I don't think we can. We scroll the visual viewport first. Once that's
maximally scrolled, we scroll the remainder in the FrameView and then fixup the
subpixel part in the visual viewport since FrameView doesn't support
fractionals. Assume our delta is positive. If we round down the FrameView
offset, the VisualViewport is maximally scrolled so it won't be able to
compensate for the fractional since it's bounded by the FrameView. We need to
"overscroll" the FrameView to ensure the pixel of which we want a fraction of is
within the FrameView. Does that make sense?

This makes sense. I suggest adding a comment (or expanding the previous one)
mentioning the 
logic behind always going to the next pixel for layout viewport.

I just noticed a bug. You are using delta.height() sign to make a decision about
both height and the width which can be incorrect. This probably does not show up
in tests
because we mainly test for vertical size changes.

[bokan, 2016-04-07 00:47:54]

On 2016/04/06 21:23:16, majidvp wrote:
> On 2016/04/06 at 19:23:35, bokan wrote:
> >
>
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> > content/renderer/render_widget.cc:1151: top_controls_shrink_blink_size_);
> > On 2016/04/06 18:07:30, majidvp wrote:
> > > I think for the top control anchoring to work correctly
> > > you also need to resize the visual viewport in the same call.
> > > This mean folding WebWidget::resizeVisualViewport into
> > > the same same resize call.
> > 
> > We already do this in WebViewImpl::performResize. When resize is called,
both
> viewports are resized. The call below to resizeVisualViewport is for the OSK
> where we might not have a layout affecting resize (i.e. resized == false
above).
> > 
> 
> Got it. Thanks for the info.
> 
> >
>
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:52: IntSize
layoutDelta
> = delta.height() < 0
> > On 2016/04/06 18:07:30, majidvp wrote:
> > > nit: Can we just round here? There does not seem to be any
> > > advantage of going to the next pixel as opposed to the closest pixel. 
> > 
> > I don't think we can. We scroll the visual viewport first. Once that's
> maximally scrolled, we scroll the remainder in the FrameView and then fixup
the
> subpixel part in the visual viewport since FrameView doesn't support
> fractionals. Assume our delta is positive. If we round down the FrameView
> offset, the VisualViewport is maximally scrolled so it won't be able to
> compensate for the fractional since it's bounded by the FrameView. We need to
> "overscroll" the FrameView to ensure the pixel of which we want a fraction of
is
> within the FrameView. Does that make sense?
> 
> This makes sense. I suggest adding a comment (or expanding the previous one)
> mentioning the 
> logic behind always going to the next pixel for layout viewport.

Done

> 
> I just noticed a bug. You are using delta.height() sign to make a decision
about
> both height and the width which can be incorrect. This probably does not show
up
> in tests
> because we mainly test for vertical size changes.

Doh! You're right, thanks! Fixed.

[majidvp, 2016-04-07 02:05:52]

On 2016/04/07 at 00:47:54, bokan wrote:
> On 2016/04/06 21:23:16, majidvp wrote:
> > On 2016/04/06 at 19:23:35, bokan wrote:
> > >
> >
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> > > File content/renderer/render_widget.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1844013002/diff/210001/content/renderer/rende...
> > > content/renderer/render_widget.cc:1151: top_controls_shrink_blink_size_);
> > > On 2016/04/06 18:07:30, majidvp wrote:
> > > > I think for the top control anchoring to work correctly
> > > > you also need to resize the visual viewport in the same call.
> > > > This mean folding WebWidget::resizeVisualViewport into
> > > > the same same resize call.
> > > 
> > > We already do this in WebViewImpl::performResize. When resize is called,
both
> > viewports are resized. The call below to resizeVisualViewport is for the OSK
> > where we might not have a layout affecting resize (i.e. resized == false
above).
> > > 
> > 
> > Got it. Thanks for the info.
> > 
> > >
> >
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/web/ResizeViewportAnchor.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1844013002/diff/210001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/web/ResizeViewportAnchor.cpp:52: IntSize
layoutDelta
> > = delta.height() < 0
> > > On 2016/04/06 18:07:30, majidvp wrote:
> > > > nit: Can we just round here? There does not seem to be any
> > > > advantage of going to the next pixel as opposed to the closest pixel. 
> > > 
> > > I don't think we can. We scroll the visual viewport first. Once that's
> > maximally scrolled, we scroll the remainder in the FrameView and then fixup
the
> > subpixel part in the visual viewport since FrameView doesn't support
> > fractionals. Assume our delta is positive. If we round down the FrameView
> > offset, the VisualViewport is maximally scrolled so it won't be able to
> > compensate for the fractional since it's bounded by the FrameView. We need
to
> > "overscroll" the FrameView to ensure the pixel of which we want a fraction
of is
> > within the FrameView. Does that make sense?
> > 
> > This makes sense. I suggest adding a comment (or expanding the previous one)
> > mentioning the 
> > logic behind always going to the next pixel for layout viewport.
> 
> Done
> 
> > 
> > I just noticed a bug. You are using delta.height() sign to make a decision
about
> > both height and the width which can be incorrect. This probably does not
show up
> > in tests
> > because we mainly test for vertical size changes.
> 
> Doh! You're right, thanks! Fixed.

lgtm

[bokan, 2016-04-07 02:09:16]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bokan, 2016-04-07 02:09:16]

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

[bokan, 2016-04-07 02:11:24]

Alexandre, --disable-threaded-scrolling causes issues with top control scrolling
that have been solved on CC. We didn't bother doing the anchoring we do in
LTHI::UpdateViewportContainerSizes on the main thread which causes clamping when
we scroll from the main thread. Adding that also necessitated doing a Blink
resize at the same time as flipping shrinks_blink_layout_size. WDYT?

[aelias_OOO_until_Jul13, 2016-04-07 02:39:54]

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1943: void
WebViewImpl::resize(const WebSize& newSize)
Does this still need to exist? Can we make all callers of WebViewImpl::resize()
call the full version?

[bokan, 2016-04-07 02:42:37]

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1943: void
WebViewImpl::resize(const WebSize& newSize)
On 2016/04/07 02:39:54, aelias wrote:
> Does this still need to exist? Can we make all callers of
WebViewImpl::resize()
> call the full version?

Unfortunately, there's lots of WebWidget types that the top controls version
doesn't apply so we have to implement it here too.

[aelias_OOO_until_Jul13, 2016-04-07 02:50:23]

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:1943: void
WebViewImpl::resize(const WebSize& newSize)
On 2016/04/07 at 02:42:37, bokan wrote:
> On 2016/04/07 02:39:54, aelias wrote:
> > Does this still need to exist? Can we make all callers of
WebViewImpl::resize()
> > call the full version?
> 
> Unfortunately, there's lots of WebWidget types that the top controls version
doesn't apply so we have to implement it here too.

Should the expanded resize() method be defined in WebView.h instead then?  And
can we give it a different name like resizeWithTopControls since overloading is
discouraged?

[bokan, 2016-04-07 23:11:08]

On 2016/04/07 02:50:23, aelias wrote:
>
https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/1844013002/diff/230001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebViewImpl.cpp:1943: void
> WebViewImpl::resize(const WebSize& newSize)
> On 2016/04/07 at 02:42:37, bokan wrote:
> > On 2016/04/07 02:39:54, aelias wrote:
> > > Does this still need to exist? Can we make all callers of
> WebViewImpl::resize()
> > > call the full version?
> > 
> > Unfortunately, there's lots of WebWidget types that the top controls version
> doesn't apply so we have to implement it here too.
> 
> Should the expanded resize() method be defined in WebView.h instead then?  And
> can we give it a different name like resizeWithTopControls since overloading
is
> discouraged?

Yah, WebView definitely makes more sense, moved that into
WebView::resizeWithTopControls as well as shifting the top controls members in
RenderWidget into RenderViewImpl.

One other issue I forgot to mention: there's still an issue with this patch
although it's less visible. If, during a scroll, we happen to commit some of the
top_controls_delta to Impl it will do its own anchoring, the deltas of which
it'll then ship back to the main thread. The problem is mostly solved in
WebViewImpl::applyScrollDeltas by turning the deltas into positions before
anchoring but I think there's some corner cases where we still end up with a
slight jump. Ideally, the Impl-thread anchoring shouldn't be added to the
SyncedScrollOffset delta but the base position so that we don't send it to main
(it'll do the anchoring correctly just from the top_controls_delta). There's no
way to do that today with SyncedProperties. Does that sound like a reasonable
thing to do?

[aelias_OOO_until_Jul13, 2016-04-07 23:24:15]

On 2016/04/07 at 23:11:08, bokan wrote:
> One other issue I forgot to mention: there's still an issue with this patch
although it's less visible. If, during a scroll, we happen to commit some of the
top_controls_delta to Impl it will do its own anchoring, the deltas of which
it'll then ship back to the main thread. The problem is mostly solved in
WebViewImpl::applyScrollDeltas by turning the deltas into positions before
anchoring but I think there's some corner cases where we still end up with a
slight jump. Ideally, the Impl-thread anchoring shouldn't be added to the
SyncedScrollOffset delta but the base position so that we don't send it to main
(it'll do the anchoring correctly just from the top_controls_delta). There's no
way to do that today with SyncedProperties. Does that sound like a reasonable
thing to do?

Hmm, I'd have to see the code needed to fix it to have a stronger opinion on
whether it's reasonable.  Is that a new regression affecting impl-thread
scrolling that would be introduced by this patch, or just another problem with
main-thread-handled scrolling?

[bokan, 2016-04-07 23:39:59]

On 2016/04/07 23:24:15, aelias wrote:
> On 2016/04/07 at 23:11:08, bokan wrote:
> > One other issue I forgot to mention: there's still an issue with this patch
> although it's less visible. If, during a scroll, we happen to commit some of
the
> top_controls_delta to Impl it will do its own anchoring, the deltas of which
> it'll then ship back to the main thread. The problem is mostly solved in
> WebViewImpl::applyScrollDeltas by turning the deltas into positions before
> anchoring but I think there's some corner cases where we still end up with a
> slight jump. Ideally, the Impl-thread anchoring shouldn't be added to the
> SyncedScrollOffset delta but the base position so that we don't send it to
main
> (it'll do the anchoring correctly just from the top_controls_delta). There's
no
> way to do that today with SyncedProperties. Does that sound like a reasonable
> thing to do?
> 
> Hmm, I'd have to see the code needed to fix it to have a stronger opinion on
> whether it's reasonable.  Is that a new regression affecting impl-thread
> scrolling that would be introduced by this patch, or just another problem with
> main-thread-handled scrolling?

It's just with main threaded handling, impl works fine for the same case (the
case is: zoom in. As you scroll up, the top controls come in, then JS scrolls
the page to the end, then you let go of the scroll and the page jumps up a few
pixels). I'll have to look into it more to figure out exactly why that's
happening but I'm pretty sure it's from the anchoring logic. Given that it's an
obscure edge case and this patch makes nothing worse, lets land this first and
I'll look into that issue in a followup. WDYT?

[aelias_OOO_until_Jul13, 2016-04-07 23:55:55]

lgtm

[bokan, 2016-04-07 23:56:58]

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

[bokan, 2016-04-07 23:56:59]

+sievers@ for content/renderer

[no sievers, 2016-04-08 19:26:02]

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:2631:
webview()->resizeWithTopControls(new_view_size,
webview() is null-checked everywhere else in OnResize() here and in
RenderWidget::OnResize() also.

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:2632: top_controls_height_,
I find it slightly weird that we end up calling resize() twice effectively here,
even if it doesn't do anything the second time.

In fact, I'm wondering if it can cause subtle bugs because when we go through
the rest of the code there is a certain order assumed for things to happen, but
here for Android we now actually resize the widget first.

When exactly (or from which call site) do you want this not to happen i.e. why
did you have to move it?

[bokan, 2016-04-11 18:32:20]

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

[commit-bot: I haz the power, 2016-04-11 18:33:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/270001

[bokan, 2016-04-11 19:54:15]

sievers@, ptal.

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:2631:
webview()->resizeWithTopControls(new_view_size,
On 2016/04/08 19:26:02, sievers wrote:
> webview() is null-checked everywhere else in OnResize() here and in
> RenderWidget::OnResize() also.

This is gone now in addressing the comment below.

https://codereview.chromium.org/1844013002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:2632: top_controls_height_,
On 2016/04/08 19:26:02, sievers wrote:
> I find it slightly weird that we end up calling resize() twice effectively
here,
> even if it doesn't do anything the second time.
> 
> In fact, I'm wondering if it can cause subtle bugs because when we go through
> the rest of the code there is a certain order assumed for things to happen,
but
> here for Android we now actually resize the widget first.
> 
> When exactly (or from which call site) do you want this not to happen i.e. why
> did you have to move it?

I moved it since we didn't want this method to exist on WebWidget since it makes
no sense there. Thus, we need access to the WebView rather than WebWidget.

You're right though we're subtly changing the order here. I've rearranged this
so that RenderViewImpl overrides a ResizeWebWidget method that gets called from
RenderWidget::Resize in the same place as before. This should be less of a
change.

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
content/renderer/render_view_impl.cc:2604:
webview()->resizeWithTopControls(new_size,
This method will early out if the parameters are unchanged so that's why I
ignore the `resized` param here. Some of the overrides of WebWidget::resize
don't early out so I kept it in RenderWidget::ResizeWebWidget but maybe that's
unnecessary as well?

[commit-bot: I haz the power, 2016-04-11 20:21:57]

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

[commit-bot: I haz the power, 2016-04-11 20:21:58]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-04-12 22:44:12]

just one nit

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
content/renderer/render_widget.cc:1095: if (resized)
Hmm, weird pattern with passing a bool to a function that tells it to not
perform the function. Can you leave that check with the caller?

[bokan, 2016-04-13 17:25:40]

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/270001/content/renderer/rende...
content/renderer/render_widget.cc:1095: if (resized)
On 2016/04/12 22:44:12, sievers wrote:
> Hmm, weird pattern with passing a bool to a function that tells it to not
> perform the function. Can you leave that check with the caller?

Agree that its odd but I'm not sure there's a clean way without doing some
rearranging. The reason I put it there is because if only the top controls
height changes, we still want to call resize in RenderView's version.
Unfortunately, RenderWidget can't check the current top control height.

I made ResizeWebWidget take the ResizeParams and make it set the various private
vars like size_ and physical_backing_size_. That way it can calculate whether a
resize occurred itself and the RenderView version can also check for a change in
top_controls_height_. I uploaded a new CL that does this. It's not ideal IMO
since there's quite a bit of duplication. The other alternative is to make more
virtual methods that RenderView overrides but then we'd need bool
NeedsWebWidgetResize(ResizeParams), void StoreResizeParams(ResizeParams),
ResizeWebWidget(ResizeParams) which seems like overkill.

Ideally, we could get rid of the resized check altogether and rely on WebWidget
implementations not to do anything unless something's changed. It seems just
plugins and popups are missing an early out right now, maybe it'd make sense to
add that there in a pre/post patch? Let me know your preference.

[no sievers, 2016-04-13 18:40:24]

thanks for your patience :)

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1096: physical_backing_size_ !=
params.physical_backing_size;
I'm wondering: If physical backing size changed to we really need to call
webwidget_->resize()? We're not passing the physical size to it, that's only
meant for the compositor, right?

This was introduced as a behavioral change in
https://codereview.chromium.org/1412663005/diff/220001/content/renderer/rende...,
not sure why, but became definitely obsolete in
https://codereview.chromium.org/1737733002/diff/420001/content/renderer/rende....

Maybe then line 1099 could also stay in the call site.

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1102: if (IsUseZoomForDSFEnabled()) {
nit: and maybe line 1102-1105 can just be a helper like:

void RenderWidget::ResizeWebWidget(const ResizeParams& params) {
  if (size_ == params.new_size)
    return;

  size_ = params.new_size;

  webwidget_->resize(GetSizeForWebWidget());
}

Then GetOriginalDeviceScaleFactor() can stay private too.

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1143: // send an ACK if we are resized to a
non-empty rect.
nit: This comment was a bit out of place even before your change. Can you move
and consolidate it with the logic in lines 1158-1163 and 1166-1167 whwere it
belongs?

[bokan, 2016-04-13 20:29:25]

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1096: physical_backing_size_ !=
params.physical_backing_size;
On 2016/04/13 18:40:24, sievers wrote:
> I'm wondering: If physical backing size changed to we really need to call
> webwidget_->resize()? We're not passing the physical size to it, that's only
> meant for the compositor, right?
> 
> This was introduced as a behavioral change in
>
https://codereview.chromium.org/1412663005/diff/220001/content/renderer/rende...,
> not sure why, but became definitely obsolete in
>
https://codereview.chromium.org/1737733002/diff/420001/content/renderer/rende....
> 
> Maybe then line 1099 could also stay in the call site.

sgtm. I'll put up a new patch with that change (and maybe put the early out into
all the WebWidget descendants) and land that first. Will put up soon...

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1102: if (IsUseZoomForDSFEnabled()) {
On 2016/04/13 18:40:23, sievers wrote:
> nit: and maybe line 1102-1105 can just be a helper like:
> 
> void RenderWidget::ResizeWebWidget(const ResizeParams& params) {
>   if (size_ == params.new_size)
>     return;
> 
>   size_ = params.new_size;
> 
>   webwidget_->resize(GetSizeForWebWidget());
> }
> 
> Then GetOriginalDeviceScaleFactor() can stay private too.

Done.

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1143: // send an ACK if we are resized to a
non-empty rect.
On 2016/04/13 18:40:23, sievers wrote:
> nit: This comment was a bit out of place even before your change. Can you move
> and consolidate it with the logic in lines 1158-1163 and 1166-1167 whwere it
> belongs?

Done.

[no sievers, 2016-04-13 23:04:31]

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1844013002/diff/290001/content/renderer/rende...
content/renderer/render_widget.cc:1096: physical_backing_size_ !=
params.physical_backing_size;
On 2016/04/13 20:29:25, bokan wrote:
> On 2016/04/13 18:40:24, sievers wrote:
> > I'm wondering: If physical backing size changed to we really need to call
> > webwidget_->resize()? We're not passing the physical size to it, that's only
> > meant for the compositor, right?
> > 
> > This was introduced as a behavioral change in
> >
>
https://codereview.chromium.org/1412663005/diff/220001/content/renderer/rende...,
> > not sure why, but became definitely obsolete in
> >
>
https://codereview.chromium.org/1737733002/diff/420001/content/renderer/rende....
> > 
> > Maybe then line 1099 could also stay in the call site.
> 
> sgtm. I'll put up a new patch with that change (and maybe put the early out
into
> all the WebWidget descendants) and land that first. Will put up soon...

ok, lgtm if you want to do that in a separate patch as a follow-up then.

[bokan, 2016-04-20 01:45:04]

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

[commit-bot: I haz the power, 2016-04-20 01:45:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/330001

[commit-bot: I haz the power, 2016-04-20 02:36:35]

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

[commit-bot: I haz the power, 2016-04-20 02:36:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-04-21 22:08:55]

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

[commit-bot: I haz the power, 2016-04-21 22:09:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844013002/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844013002/350001

[commit-bot: I haz the power, 2016-04-22 01:53:45]

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

[commit-bot: I haz the power, 2016-04-22 01:53:47]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-04-27 12:38:15]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-04-27 12:38:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, aelias@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1844013002/#ps370001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-27 12:38:27]

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

[commit-bot: I haz the power, 2016-04-27 12:56:04]

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

[commit-bot: I haz the power, 2016-04-27 12:56:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[bokan, 2016-04-27 14:10:37]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-04-27 14:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
majidvp@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1844013002/#ps390001
(title: "Build fix after rebase")

[commit-bot: I haz the power, 2016-04-27 14:10:58]

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

[commit-bot: I haz the power, 2016-04-27 15:49:22]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-27 15:49:23]

Committed patchset #20 (id:390001)

[commit-bot: I haz the power, 2016-04-30 17:10:08]

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

[commit-bot: I haz the power, 2016-04-30 17:50:08]

Description was changed from

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG=600507
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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