I think cc::Layer::position() is no longer meaningful with layer lists. Should we switch to offset_to_transform_parent() for these tests?

I think we should but, to do that cleanly, should we refactor cc::Layer a bit?

For example, cc::Layer::offset_to_transform_parent has a comment about it being an internal property tree builder detail, and it does look like it is cleared in the non-layer-list codepath at the end of PropertyTreeBuilderContext::AddTransformNodeIfNeeded. Maybe we could DCHECK that cc::Layer::position is only get/set when not using layer lists?

Re #c3: SGTM.

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

commit a4a4d92f73fa5d55428301ac660720b2509dfbab
Author: Philip Rogers <pdr@chromium.org>
Date: Sat Jan 05 16:00:04 2019

Remove support for root cc::Layer position

TransformTree::SetRootTransformsAndScales has special handling of the
root layer's position. This function is used for the layer list and non-
layer-list codepaths. In the layer-list codepath we would like to remove
cc::Layer::position. In support of that, this patch removes the unneeded
handling of root cc::Layer position and DCHECKs that it is not set in
non-test scenarios.

This removes the following test:
LayerTreeHostCommonTest::VisibleRectsForPositionedRootLayerClippedByViewport
This was only added for testing in https://crrev.com/333109.

Bug:  911664 
Change-Id: Ic2062fb3071280cbe76cb2b2b76e9a861ce31ff3
Reviewed-on: https://chromium-review.googlesource.com/c/1396789
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620184}
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/layer_tree_impl_unittest.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/property_tree.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/property_tree.h
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/property_tree_unittest.cc
[modify] https://crrev.com/a4a4d92f73fa5d55428301ac660720b2509dfbab/cc/trees/tree_synchronizer_unittest.cc

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

commit 40c286acc416ea3247743c1ec3dae31702c1943a
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Jan 07 16:40:49 2019

[BlinkGenPropertyTrees] Stop using layer position for viewport scrollbars

Both cc::Layer::position and cc::Layer::offset_to_transform_parent (via
GraphicsLayer::SetLayerState) are used for positioning scrollbars in
visual_viewport.cc. Pre-BlinkGenPropertyTrees (BGPT), cc's property tree builder
used cc::Layer::position to set cc::Layer::offset_to_transform_parent. With BGPT
cc::Layer::position is not needed and cc::Layer::offset_to_transform_parent can
be set directly (through GraphicsLayer::SetLayerState). This patch stops using
cc::Layer::SetPosition for BGPT.

This patch has two important parts:
1) The calculations for ScrollbarThickness, ScrollbarSize, and ScrollbarOffset
have been extracted out of VisualViewport::SetupScrollbar.
2) ScrollbarOffset is used in VisualViewport::UpdatePaintPropertyNodesIfNeeded
instead of reading the value from cc::Layer's position.

For debugging, Layer::ToString now lists offset_to_transform_parent because the
position field may not be set with BGPT.

Bug:  911664 
Change-Id: I936469091adcdf0760ff81a2d4bea7cfea9ae104
Reviewed-on: https://chromium-review.googlesource.com/c/1396691
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620350}
[modify] https://crrev.com/40c286acc416ea3247743c1ec3dae31702c1943a/cc/layers/layer.cc
[modify] https://crrev.com/40c286acc416ea3247743c1ec3dae31702c1943a/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/40c286acc416ea3247743c1ec3dae31702c1943a/third_party/blink/renderer/core/frame/visual_viewport.h

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

commit e459da404c2890d7fe86fdf00802dcc52e0068c1
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jan 08 00:16:45 2019

Do not cache GraphicsLayer's contents offset to transform node

In https://crrev.com/571989 we started caching both a property tree
state and an offset for the contents layer. This patch stops caching the
offset because the value can be calculated from a existing offsets
(GraphicsLayer's GetOffsetFromTransformNode() and |contents_rect_|).

The motivation for this patch is to stop using cc::Layer::position
as a temporary store of data for BlinkGenPropertyTrees (BGPT). cc::Layer
position is only needed for building property trees and BGPT can
directly set the cc::Layer's OffsetToTransformParent. With this patch,
one more BGPT callsite of cc::Layer::position has been removed.

Bug:  911664 
Change-Id: I4bc80c7ed1bcf81d18af72263e02e59c21a03ac5
Reviewed-on: https://chromium-review.googlesource.com/c/1398064
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620545}
[modify] https://crrev.com/e459da404c2890d7fe86fdf00802dcc52e0068c1/third_party/blink/renderer/core/paint/compositing/compositing_layer_property_updater.cc
[modify] https://crrev.com/e459da404c2890d7fe86fdf00802dcc52e0068c1/third_party/blink/renderer/platform/graphics/graphics_layer.cc
[modify] https://crrev.com/e459da404c2890d7fe86fdf00802dcc52e0068c1/third_party/blink/renderer/platform/graphics/graphics_layer.h

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

commit f857b2f2e1733fea2dfc193b3afdf52f90287277
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jan 08 01:35:36 2019

Remove cc::LayerImpl::position

cc::Layer has a position field that is used when building property trees
in cc to set cc::Layer::offset_to_transform_parent. cc::Layer's
offset_to_transform_parent is then copied to cc::LayerImpl and is used
as cc::LayerImpl's offset. There is no need for a position field on
cc::LayerImpl so this patch removes it.

Comments above cc::Layer's |position| and |offset_to_transform_parent|
have been clarified.

Bug:  911664 
Change-Id: I4469c0153e27753d64809ee40021ce68ecefe3af
Reviewed-on: https://chromium-review.googlesource.com/c/1396384
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620570}
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/input/scrollbar_animation_controller_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/input/single_scrollbar_animation_controller_thinning_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/effect_tree_layer_list_iterator_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer.h
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer_impl.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer_impl.h
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer_impl_test_properties.h
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/layer_impl_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/layers/picture_layer_impl_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/test/layer_test_common.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/test/layer_tree_json_parser.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/test/layer_tree_json_parser_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/damage_tracker_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_host_unittest_scroll.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/layer_tree_impl_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/occlusion_tracker_unittest.cc
[modify] https://crrev.com/f857b2f2e1733fea2dfc193b3afdf52f90287277/cc/trees/property_tree_builder.cc

The real reason this test is different is due to the non-BGPT codepath creating an Ancestor Clipping Layer which offsets the hit-test-containing layer.

Without BGPT:
"name": "Scrolling Contents Layer",
"bounds": [800, 600],
"contentsOpaque": true,
"backgroundColor": "#FFFFFF",
"flattenInheritedTransform": false,
"children": [
  {
    "name": "Ancestor Clipping Layer",
    "position": [14, 9],
    "bounds": [273, 100],
    "drawsContent": false,
    "shouldFlattenTransform": false,
    "children": [
      {
        "name": "LayoutBlockFlow DIV id='compositedLayer'",
        "position": [0, 10],
        "bounds": [273, 22],
        "transform": [
          [1, 0, 0, 0],
          [0, 1, 0, 0],
          [0, 0, 1, 0],
          [0, 0, 1, 1]
        ],
        "flattenInheritedTransform": false
      }
    ]
  }
]


With BGPT:
"name": "Scrolling Contents Layer",
"bounds": [800, 600],
"contentsOpaque": true,
"backgroundColor": "#FFFFFF",
"flattenInheritedTransform": false,
"children": [
  {
    "name": "LayoutBlockFlow DIV id='compositedLayer'",
    "position": [14, 19],
    "bounds": [273, 22],
    "transform": [
      [1, 0, 0, 0],
      [0, 1, 0, 0],
      [0, 0, 1, 0],
      [0, 0, 1, 1]
    ]
  }
]

This test can safely be rebaselined.

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

commit 7f85f3f7f7611fbb8df1f879b7448a7514446da2
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jan 08 21:23:18 2019

Rebaseline compositor-touch-hit-rects-non-composited-scroll.html

This test is different with BlinkGenPropertyTrees (BGPT) because BGPT
does not create an ancestor overflow clip layer. This additional layer
causes the layer's position to be different with BGPT. The actual
hit test rect is correct.

Bug:  911664 
Change-Id: I5e00e5566370453491f7359685ec5ffc316cffdb
Reviewed-on: https://chromium-review.googlesource.com/c/1401264
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620882}
[modify] https://crrev.com/7f85f3f7f7611fbb8df1f879b7448a7514446da2/third_party/blink/web_tests/TestExpectations
[modify] https://crrev.com/7f85f3f7f7611fbb8df1f879b7448a7514446da2/third_party/blink/web_tests/fast/events/touch/compositor-touch-hit-rects-non-composited-scroll-expected.txt

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

commit a1afb1b2c7fb2ba928588d868e247dd7e7a79a6a
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jan 08 22:21:25 2019

Fix link highlight raster invalidation position

Link highlight raster invalidation incorrectly used the layer's position. The
invalidations should be in the space of the layer.

With this patch, one more callsite of cc::Layer::position has been removed.

Bug:  911664 
Change-Id: I38fc47b3f63b9ca5360cd46048a3c609b52d5fe9
Reviewed-on: https://chromium-review.googlesource.com/c/1400944
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620905}
[modify] https://crrev.com/a1afb1b2c7fb2ba928588d868e247dd7e7a79a6a/third_party/blink/renderer/core/frame/link_highlights.h
[modify] https://crrev.com/a1afb1b2c7fb2ba928588d868e247dd7e7a79a6a/third_party/blink/renderer/core/paint/link_highlight_impl.cc
[modify] https://crrev.com/a1afb1b2c7fb2ba928588d868e247dd7e7a79a6a/third_party/blink/renderer/core/paint/link_highlight_impl_test.cc

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

commit c511695d39c66782d29f51c1bd7b2457dc8634b8
Author: Philip Rogers <pdr@chromium.org>
Date: Wed Jan 09 00:21:24 2019

Continue using cc::Layer::SetPosition with BlinkGenPropertyTrees

Because the inspector still has cc::Layer::position dependencies (see:
https://crbug.com/916768), we need to continue setting
cc::Layer::SetPosition with BlinkGenPropertyTrees. This patch restores
two callsites that were skipped in https://crrev.com/620350 and
https://crrev.com/620545.

Bug:  911664 , 916768
Change-Id: Ie715282b40e40a51002adf07e933ff31b1e8f2fe
Reviewed-on: https://chromium-review.googlesource.com/c/1401233
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620956}
[modify] https://crrev.com/c511695d39c66782d29f51c1bd7b2457dc8634b8/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/c511695d39c66782d29f51c1bd7b2457dc8634b8/third_party/blink/renderer/platform/graphics/graphics_layer.cc

This is pretty much fixed.

Here's a summary:
cc::Layer::position is needed for two things: the inspector layer tree view which is based on GraphicsLayers (https://crbug.com/916768) and cc's property tree builder. With BlinkGenPropertyTrees, cc's property tree builder is not used so the only need for cc::Layer::position is for the inspector. To make the inspector work with CompositeAfterPaint, this dependency is being removed.

The purpose of cc::Layer::position pre-BGPT (aka pre-IsUsingLayerLists) is to set cc::Layer::offset_to_transform_parent which is then used to update cc::LayerImpl::offset_to_transform_parent. With BGPT (aka IsUsingLayerLists), we set offset_to_transform_parent directly.

It is not immediately obvious that we can do this to cc::Layer::position with BGPT once the inspector dependency is removed (https://crbug.com/916768):
const gfx::PointF& position() const {
  DCHECK(!layer_tree_host() || !layer_tree_host()->IsUsingLayerLists());
  return inputs_.position;
}
This is possible because all of the remaining callsites to GraphicsLayer::GetPosition are actually only used to call GraphicsLayer::SetPosition.