End-to-end prototype of compositor scrolling with slimming paint v2

End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2. This patch has two large changes (which can be split up):

1) cc->blink: scroll offset callbacks now go straight to the associated
ScrollableArea, bypassing GraphicsLayers which are not used in SPV2. This
is done by moving didScroll (called by Layer::SetScrollOffsetFromImplSide)
from GraphicsLayer to ScrollableArea. Instead of having the GraphicsLayer
callback lookup the new scroll offset and pass it to ScrollableArea, the
scroll offset is now passed as a parameter in the callback.

This approach lets us share the cc->blink logic between SPV1 and SPV2, as
well as avoiding looking up the ScrollableArea from the cc::scroll_node's
compositor element id.

2) blink->cc: PaintArtifactCompositor now sets both the scroll tree's
scroll offset and the cc scroll_node's owning_layer_id (temporarily,
until cc's scroll node is refactored to not need it). The SPV2 didScroll
callback (linking Blink's ScrollableArea to cc's Layer) is also set
through PaintArtifactCompositor::update, though there may be a cleaner
place for setting this compositor-related information which will also
include touch event regions.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[pdr., 2017-02-04 02:08:49]

Description was changed from

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
==========

to

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-04 02:08:50]

Description was changed from

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[pdr., 2017-02-04 02:12:58]

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

[commit-bot: I haz the power, 2017-02-04 02:13:17]

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

[pdr., 2017-02-04 02:13:53]

Description was changed from

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-04 02:14:05]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2017-02-06 07:31:30]

Description was changed from

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2.

The scroll tree scroll offset is now set by PaintArtifactCompositor. Scroll
nodes also have their correct owning_layer_id values set (temporarily,
until cc's scroll node is refactored to not need it). Lastly, ScrollableAreas
are wired up to receive didScroll(ScrollOffset) events from the impl side.
This approach lets us avoid having to map from CompositorElementId to
ScrollableArea on scroll events, and unifies SPV1 and SPV2 to use the
same didScroll callback.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
End-to-end prototype of compositor scrolling with slimming paint v2

This is an end-to-end prototype of compositor scrolling with slimming
paint v2. This patch has two large changes (which can be split up):

1) cc->blink: scroll offset callbacks now go straight to the associated
ScrollableArea, bypassing GraphicsLayers which are not used in SPV2. This
is done by moving didScroll (called by Layer::SetScrollOffsetFromImplSide)
from GraphicsLayer to ScrollableArea. Instead of having the GraphicsLayer
callback lookup the new scroll offset and pass it to ScrollableArea, the
scroll offset is now passed as a parameter in the callback.

This approach lets us share the cc->blink logic between SPV1 and SPV2, as
well as avoiding looking up the ScrollableArea from the cc::scroll_node's
compositor element id.

2) blink->cc: PaintArtifactCompositor now sets both the scroll tree's
scroll offset and the cc scroll_node's owning_layer_id (temporarily,
until cc's scroll node is refactored to not need it). The SPV2 didScroll
callback (linking Blink's ScrollableArea to cc's Layer) is also set
through PaintArtifactCompositor::update, though there may be a cleaner
place for setting this compositor-related information which will also
include touch event regions.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-06 07:31:30]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org, wkorman@chromium.org

[pdr., 2017-02-06 07:33:21]

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

[commit-bot: I haz the power, 2017-02-06 07:33:39]

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, 2017-02-06 09:08:07]

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

[commit-bot: I haz the power, 2017-02-06 09:08:08]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-06 21:28:52]

This looks great! Splitting into two changes sounds like a good idea. When you
do that, one comment about the change to cc->blink plumbing:

https://codereview.chromium.org/2676923002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/2676923002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:633: void DidScroll(const
gfx::ScrollOffset&) {
Perhaps this (or one of the other tests in this file) could check that the
offset passed in has the right value?

[pdr., 2017-02-07 22:14:34]

https://codereview.chromium.org/2676923002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/2676923002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:633: void DidScroll(const
gfx::ScrollOffset&) {
I talked offline with both Ali and Walter and both seem cautiously optimistic
about this approach. I've gone ahead and split out the largest (and most
mechanical) part which moves didScroll to ScrollableArea:
https://codereview.chromium.org/2680953002


On 2017/02/06 at 21:28:52, ajuma wrote:
> Perhaps this (or one of the other tests in this file) could check that the
offset passed in has the right value?

Good idea, done in the split-out patch.

[pdr., 2017-02-18 18:31:43]

Closing as this as been landed with:

Remove GraphicsLayer::didScroll and directly call ScrollableArea::didScroll
https://codereview.chromium.org/2680953002

Set layer scroll data from PaintArtifactCompositor
https://codereview.chromium.org/2698473006