Made layout viewport scroll updates from compositor work like ordinary layers.

Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

BUG=505516
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4222c27d55d1de9a3d8a13070266e2c8517aead5
Cr-Commit-Position: refs/heads/master@{#406022}

[bokan, 2016-07-14 14:58:39]

Description was changed from

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

BUG=505516
==========

to

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

[bokan, 2016-07-14 15:00:14]

Description was changed from

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

to

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

[bokan, 2016-07-14 15:00:15]

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

[bokan, 2016-07-14 15:00:58]

Since aelias@ is out, enne@, are you a good reviewer for this?

[bokan, 2016-07-14 15:51:53]

https://codereview.chromium.org/2144303002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (left):

https://codereview.chromium.org/2144303002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:1181: //
crbug.com/437620
This test was testing a main-thread mitigation to an issue with the compositor,
that it applied a counter scroll to a top controls animation to the outer
viewport first. This is no longer the case so this test isn't actually doing
anything.

[enne (OOO), 2016-07-14 18:56:04]

lgtm in general

This all seems pretty straightforward.

https://codereview.chromium.org/2144303002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/2144303002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:162: inner_viewport_scroll.layer_id =
Layer::INVALID_ID;
Maybe ScrollUpdateInfo's ctor should do this instead?

[bokan, 2016-07-14 21:41:10]

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

[bokan, 2016-07-14 21:41:24]

+jbroman@ for platform/graphics

https://codereview.chromium.org/2144303002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/2144303002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:162: inner_viewport_scroll.layer_id =
Layer::INVALID_ID;
On 2016/07/14 18:56:04, enne wrote:
> Maybe ScrollUpdateInfo's ctor should do this instead?

Done.

[bokan, 2016-07-18 14:45:25]

jbroman@: ping

[jbroman, 2016-07-18 14:48:14]

platform lgtm (thanks for the ping; sorry for not noticing this CL earlier)

[bokan, 2016-07-18 14:52:19]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-07-18 14:52:19]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2144303002/#ps60001
(title: "Initialize ScrollUpdateInfo's layer id in constructor")

[commit-bot: I haz the power, 2016-07-18 14:52:44]

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 14:52:46]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

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

Description was changed from

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

to

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

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

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

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

to

==========
Made layout viewport scroll updates from compositor work like ordinary layers.

Most scroll layers in CC update the main thread with changed scroll offsets by
using the did_scroll_callback, which calls into GraphicsLayer to update its
copy of the scroll offset. The exception to this has been viewport layers,
which don't set a did_scroll_callback. Instead, the viewport gets its values
applied in WebViewImpl::applyViewportDeltas.  This was done since viewport
scroll bounds were dependent on page scale and top controls offset so these
values had to be committed atomically.

It turns out, only the inner (visual) viewport's scroll bounds depend on the
page scale. While the outer (layout) viewport's scroll bounds do depend on top
controls, the compositor already clamps the scroll offset to the acceptable
bounds so as long as we apply the top controls delta after the scroll offset
(to prevent the main thread from clamping it again) the outer viewport doesn't
need to be in applyViewportDeltas.

This CL removes the layout viewport as part of applyViewportDeltas and makes it
use the regular did_scroll_callback mechanism. This simplifies the root
scroller implementation which can change which layer is the outer viewport.
Having the outer viewport use the callback avoids some race conditions as to
which layer scroll gets applied to during a change of outer viewport.

I've left the parameter in applyViewportDeltas' signature since that's a large
mechanical change that'll touch many files. This Cl just passes 0 for the
layout viewport scroll delta and a future CL will remove the parameter itself.

BUG=505516
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4222c27d55d1de9a3d8a13070266e2c8517aead5
Cr-Commit-Position: refs/heads/master@{#406022}
==========

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

Patchset 4 (id:??) landed as
https://crrev.com/4222c27d55d1de9a3d8a13070266e2c8517aead5
Cr-Commit-Position: refs/heads/master@{#406022}