I have a patch [1] that force-enables BlinkGenPropertyTrees and found several tests that fail[2] due to not having viewport layers. We should make sure these tests pass:
WheelScrollLatchingBrowserTest.WheelScrollingRelatchWhenLatchedScrollerRemoved
BrowserSideFlingBrowserTest.WheelScrollingWorksAfterAutoscrollCancel
AsyncWheelEventsBrowserTest.WheelScrollingRelatchWhenLatchedScrollerRemoved
CompositorEventAckBrowserTest.TouchStart
TouchSelectionForCrossProcessFramesTests/TouchSelectionControllerClientAuraSiteIsolationTest.BasicSelectionIsolatedScrollMainframe/0
TouchActionBrowserTest.PanXMainThreadJanky
SitePerProcessBrowserTest.ScrollLocalSubframeInOOPIF
AsyncWheelEventsBrowserTest.WheelEventTarget
CompositorEventAckBrowserTest.TouchStartDuringFling
TouchActionBrowserTest.DefaultAuto

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1058604
[2] https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/98353

I think Tien-Ren is looking at this now, as it is our top crasher for correctness.

Tien-Ren, can you share your progress/plans? I've got a patch in progress on making pinch-zoom work so we should sync up. Right now, I'm trying to create a transform property node to replace the page scale layer.

Ah, I was investigating the same thing. I was looking at the 6 viewport layers and thinking how could I migrate their functionality to property nodes.

Since you already have a patch in progress, you are ahead of me. I can review your patch later when it's ready!

Tien-Ren and I had discussed making PaintPropertyTreeBuilder take as input the root properties. Then, whoever creates the root layers & root properties (maybe WebViewImpl?) would pass them into PaintPropertyTreeBuilder. I think WebViewImpl is the owner of all the additional layers such as overlays, devtools, etc. That's as far as I got thinking about this problem.

My approach so far has been to hang the required nodes on the VisualViewport (i.e. it owns the nodes - associates them with the VisualViewport ElementId) and add a VisualViewportPaintPropertyTreeBuilder to add the nodes during PrePaint, in analogue to FrameViewPaintPropertyTreeBuilder (though maybe a whole builder is overkill...). Does that sound reasonable?

I'm currently working on removing uses of the page scale layer on the CC-side and plumbing through the VisualViewport ElementId so we can lookup the transform node without going through the layer.

One question I do have and one of you might be able to answer: do we need to have a separate transform nodes for the scale and the translation? i.e. can a TransformNode associated with a ScrollNode have an arbitrary transform matrix or is it limited to just 2d translation?

Your approach sounds reasonable to me. Will PaintPropertyTreeBuilder call into VisualViewportPaintPropertyTreeBuilder?

With your approach, can you think of a way to hook up the WebViewImpl layers to the property trees? WebViewImpl has a few special layers (see: WebViewImpl::UpdateLifecycle) and we'll need to attach them to the correct property nodes somehow. We shouldn't block your approach on solving this too, just something to keep in mind.

I can answer your question: the scroll transform node is different in blink and cc. In blink, we have a full transform. In cc, we only have a 2d offset. In PropertyTreeManager::EnsureCompositorTransformNode (which is called from PaintArtifactCompositor), we flatten blink's scroll transform into just a 2d offset and set it on cc's scroll transform node. It is possible to change how blink creates/updates cc's scroll transform, but I'd recommend starting with an approach with a transform node just for scale.

Yea, I'm more inclined to have a VisualViewportPaintPropertyTreeBuilder class, but I think there is nothing wrong for it to be a part of VisualViewport or PrePaintTreeWalk class.

Is it more convenient to lookup through ElementId? Would it be easier if we just tell cc the node index?

There is no real technical reason why we can't use a single transform node for both scrolling translation and scaling. It is just that because translation and scaling doesn't commute, we must clearly define the order of operations. Keeping them separate makes code clearer.

> Will PaintPropertyTreeBuilder call into VisualViewportPaintPropertyTreeBuilder?

yes

> can you think of a way to hook up the WebViewImpl layers to the property trees

I think the stuff in WebView is all just content overlays (link highlights, dev tools overlay, etc.) so I'm guessing they don't actually need their own property tree nodes but should just be associated with the same nodes as the main LayoutView (since they're all relative to content). That said, it wouldn't surprise me if this required some untangling (e.g. if devtools today calculates the page scale and sizes/positions the overlay layers itself). I think we could just add a step at the end of the PrePaint walk that hooks these up, similar to how WebViewImpl::UpdateLifecycle tacks these on at the end. Alternatively, we could do it when we walk the main LayoutView. Longer term we'll probably want these kinds of things painted from within Blink's painting system, right?

> ... I'd recommend starting with an approach with a transform node just for scale.
> ... Keeping them separate makes code clearer.

Thanks, I'll do that.

> Is it more convenient to lookup through ElementId? Would it be easier if we just tell cc the node index?

It's no easier/harder and seemed to me more appropriate to use the ElementId (e.g. is it possible the node indices may change as a result of a rebuild? The viewport ElementId I know should remain stable). The one wrinkle is that we'll need to refer to two separate transform nodes for the visual viewport - I noticed there's a ElementIdNamespace and figured that might be a way. Would this be a valid use of ElementIdNamespace or would it be abusing the concept?

If you think the node id is a better choice here I can do that, it's basically the exact same plumbing.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6357cc72c771e3f40f610bed88db805b67580a00

commit 6357cc72c771e3f40f610bed88db805b67580a00
Author: David Bokan <bokan@chromium.org>
Date: Mon Jun 11 01:09:52 2018

[BlinkGenPropTree] Stop using PageScaleLayer in cc

This patch prepares CC for turning on blink-generated-property-trees by
removing pinch-zoom/page-scale related uses of the PageScaleLayer. The
only remaining uses are in the cc property tree builder which is only
used when BGPT is disabled. When BGPT is enabled, Blink will create a
page scale transform node rather than a layer. Without BGPT, we find the
appropriate transform node from the PageScaleLayer.

The main change in this CL is making UpdatePageScale work on a property
tree node rather than Layer/LayerImpl. The one complication here is
that there exists a mode for the compositor where the page scale is the
root layer. In this mode, the device scale factor, page scale factor,
and device transform are all applied to the same node. This mode is
used by the UI compositor which isn't yet running BGPT so this use and
building the trees are the only remaining uses of the page scale layer.

This change should have no observable effects.

Bug:  836910 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I87b251e79fb0b89e3cc5f088bebdf862b22c3e6c
Reviewed-on: https://chromium-review.googlesource.com/1079608
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565915}
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/draw_property_utils.h
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_host_common.h
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_host_common_perftest.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/6357cc72c771e3f40f610bed88db805b67580a00/components/viz/service/display/bsp_tree_perftest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c40a5703e3451b97993c936cd69094814444a417

commit c40a5703e3451b97993c936cd69094814444a417
Author: David Bokan <bokan@chromium.org>
Date: Tue Jun 12 13:01:19 2018

[BlinkGenPropTree] Use of viewport scroll node instead of layer

Similar to https://crrev.com/c/1079608, this CL replaces uses of the
viewport scroll layers with the equivalent scroll node from the property
tree. This prepares for enabling blink-generated-property-trees since we
wont create the viewport scroll layers in that case, only the property
tree nodes.

This patch makes call sites use an accessor method which grabs the
scroll node from the viewport layer. A follow up patch will change this
helper to use the property tree directly if we're in BGPT mode. Some
more involved uses of the viewport layers remain and will have to be
fixed in future CLs but this is enough to enable basic scale-and-scroll
funcionality.

Note: while we did convert some uses of the outer viewport layer here,
we're mainly concerned with the inner viewport. That's because the
outer viewport is a scroller that generates content (like other
scrollers) and will continue to create and use a layer in BGPT. The
inner viewport layer is conceptual and will thus not generate a layer.

This CL should have no functional changes.

Bug:  836910 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ifa83fed2fa9b1847729ffafe75846da08e204237
Reviewed-on: https://chromium-review.googlesource.com/1085639
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566393}
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/input/browser_controls_offset_manager.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/input/browser_controls_offset_manager_client.h
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/input/browser_controls_offset_manager_unittest.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/input/scroll_elasticity_helper.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/layers/viewport.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/layers/viewport.h
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/property_tree.cc
[modify] https://crrev.com/c40a5703e3451b97993c936cd69094814444a417/cc/trees/property_tree.h

 Issue 846771  has been merged into this issue.