Cleanup the Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2877673002
Cr-Commit-Position: refs/heads/master@{#471490}

Author: progers

cc/layers/layer.cc

@@ -771,17 +771,7 @@ void Layer::SetScrollOffset(const gfx::ScrollOffset& scroll_offset) {
   if (!layer_tree_host_)
     return;
 
-  PropertyTrees* property_trees = layer_tree_host_->property_trees();
-  if (scroll_tree_index() != ScrollTree::kInvalidNodeId && scrollable())
-    property_trees->scroll_tree.SetScrollOffset(id(), scroll_offset);
-
-  if (TransformNode* transform_node =
-          property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
-    DCHECK_EQ(transform_tree_index(), transform_node->id);
-    transform_node->scroll_offset = CurrentScrollOffset();
-    transform_node->needs_local_transform_update = true;
-    property_trees->transform_tree.set_needs_update(true);
-  }
+  UpdateScrollOffset(scroll_offset);
 
   SetNeedsCommit();
 }
@@ -797,17 +787,7 @@ void Layer::SetScrollOffsetFromImplSide(
   inputs_.scroll_offset = scroll_offset;
   SetNeedsPushProperties();
 
-  PropertyTrees* property_trees = layer_tree_host_->property_trees();
-  if (scroll_tree_index() != ScrollTree::kInvalidNodeId && scrollable())
-    property_trees->scroll_tree.SetScrollOffset(id(), scroll_offset);
-
-  if (TransformNode* transform_node =
-          property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
-    DCHECK_EQ(transform_tree_index(), transform_node->id);
-    transform_node->scroll_offset = CurrentScrollOffset();
-    transform_node->needs_local_transform_update = true;
-    property_trees->transform_tree.set_needs_update(true);
-  }
+  UpdateScrollOffset(scroll_offset);
 
   if (!inputs_.did_scroll_callback.is_null())
     inputs_.did_scroll_callback.Run(scroll_offset);
@@ -816,6 +796,28 @@ void Layer::SetScrollOffsetFromImplSide(
   // "this" may have been destroyed during the process.
 }
 
+void Layer::UpdateScrollOffset(const gfx::ScrollOffset& scroll_offset) {
+  DCHECK(scrollable());
+  if (scroll_tree_index() == ScrollTree::kInvalidNodeId) {
+    // Ensure the property trees just have not been built yet but are marked for
+    // being built which will set the correct scroll offset values.
+    DCHECK(layer_tree_host_->property_trees()->needs_rebuild);
+    return;
+  }
+
+  // If a scroll node exists, it should have an associated transform node.
+  DCHECK(transform_tree_index() != TransformTree::kInvalidNodeId);
+
+  auto& property_trees = *layer_tree_host_->property_trees();
+  property_trees.scroll_tree.SetScrollOffset(id(), scroll_offset);
+  auto* transform_node =
+      property_trees.transform_tree.Node(transform_tree_index());
+  DCHECK_EQ(transform_tree_index(), transform_node->id);
+  transform_node->scroll_offset = CurrentScrollOffset();
+  transform_node->needs_local_transform_update = true;
+  property_trees.transform_tree.set_needs_update(true);
+}
+
 void Layer::SetScrollClipLayerId(int clip_layer_id) {
   DCHECK(IsPropertyChangeAllowed());
   if (inputs_.scroll_clip_layer_id == clip_layer_id)

cc/layers/layer.h

@@ -502,6 +502,12 @@ class CC_EXPORT Layer : public base::RefCounted<Layer> {
   // layer should own a property tree node or not.
   void SetPropertyTreesNeedRebuild();
 
+  // Fast-path for |SetScrollOffset| and |SetScrollOffsetFromImplSide| to
+  // directly update scroll offset values in the property tree without needing a
+  // full property tree update. If property trees do not exist yet, ensures
+  // they are marked as needing to be rebuilt.
+  void UpdateScrollOffset(const gfx::ScrollOffset&);
+
   // Encapsulates all data, callbacks or interfaces received from the embedder.
   // TODO(khushalsagar): This is only valid when PropertyTrees are built
   // internally in cc. Update this for the SPv2 path where blink generates

cc/layers/scrollbar_layer_unittest.cc

@@ -437,6 +437,7 @@ TEST_F(ScrollbarLayerTest, ThumbRect) {
 
   // Over-scroll (thumb position should clamp on the far side).
   root_layer->SetScrollOffset(gfx::ScrollOffset(85, 0));
+  layer_tree_host_->UpdateLayers();
 
   UPDATE_AND_EXTRACT_LAYER_POINTERS();
   EXPECT_EQ(gfx::Rect(56, 0, 4, 10).ToString(),
@@ -909,6 +910,7 @@ class ScrollbarLayerTestResourceCreationAndRelease : public ScrollbarLayerTest {
                           int expected_deleted,
                           bool use_solid_color_scrollbar) {
     std::unique_ptr<Scrollbar> scrollbar(new FakeScrollbar(false, true, false));
+    scoped_refptr<Layer> root_clip_layer = Layer::Create();
     scoped_refptr<Layer> layer_tree_root = Layer::Create();
     scoped_refptr<Layer> content_layer = Layer::Create();
     scoped_refptr<Layer> scrollbar_layer;
@@ -930,6 +932,7 @@ class ScrollbarLayerTestResourceCreationAndRelease : public ScrollbarLayerTest {
 
     scrollbar_layer->SetIsDrawable(true);
     scrollbar_layer->SetBounds(gfx::Size(100, 100));
+    layer_tree_root->SetScrollClipLayerId(root_clip_layer->id());
     layer_tree_root->SetScrollOffset(gfx::ScrollOffset(10, 20));
     layer_tree_root->SetBounds(gfx::Size(100, 200));
     content_layer->SetBounds(gfx::Size(100, 200));

third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp

@@ -770,6 +770,9 @@ TEST_F(PaintArtifactCompositorTestWithPropertyTrees, OneScrollNode) {
   EXPECT_EQ(transform_node_index, transform_node.id);
 
   EXPECT_EQ(0u, scroll_client.did_scroll_count);
+  // TODO(pdr): The PaintArtifactCompositor should set the scroll clip layer id
+  // so the Layer is scrollable. This call should be removed.
+  layer->SetScrollClipLayerId(layer->id());
   layer->SetScrollOffsetFromImplSide(gfx::ScrollOffset(1, 2));
   EXPECT_EQ(1u, scroll_client.did_scroll_count);
   EXPECT_EQ(gfx::ScrollOffset(1, 2), scroll_client.last_scroll_offset);