Rewrite Layer::SetScrollOffset{FromImplSide} property tree fast-path

cc/layers/layer.cc

Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 6845f0e111c893527bec976f40f4d929b911ae45..ee9580fda59e92804f7b2b247418e834138f6a21 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -776,16 +776,11 @@ 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);
+  DCHECK(scrollable());
 
-  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);
+  if (!UpdateExistingScrollOffsetPropertyTreeNodes(scroll_offset)) {
+    // Ensure property trees will be updated which will set the scroll offset.
+    DCHECK(layer_tree_host_->property_trees()->needs_rebuild);
   }
 
   SetNeedsCommit();
@@ -802,16 +797,11 @@ 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);
+  DCHECK(scrollable());
 
-  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);
+  if (!UpdateExistingScrollOffsetPropertyTreeNodes(scroll_offset)) {
+    // Ensure property trees will be updated which will set the scroll offset.
+    DCHECK(layer_tree_host_->property_trees()->needs_rebuild);
   }
 
   if (!inputs_.did_scroll_callback.is_null())
@@ -821,6 +811,25 @@ void Layer::SetScrollOffsetFromImplSide(
   // "this" may have been destroyed during the process.
 }
 
+bool Layer::UpdateExistingScrollOffsetPropertyTreeNodes(

[wkorman, 2017/05/11 17:42:13]

Add unit test for this?

[pdr., 2017/05/11 18:03:37]

This function is really just a refactoring of the common functionality from
SetScrollOffset and SetScrollOffsetFromImplSide into a helper. This area is
tested in many tests. Do you think we need an additional test for just the
helper?

[wkorman, 2017/05/11 18:21:37]

It looks like a public method on layer, so I would say yes, but I am not an
OWNER and much less familiar with scroll logic so am fine with your thought.

+    const gfx::ScrollOffset& scroll_offset) {
+  if (scroll_tree_index() == ScrollTree::kInvalidNodeId)
+    return false;

[wkorman, 2017/05/11 17:42:13]

Why not put DCHECK in two places above in here, and then do we still need
retval?

[pdr., 2017/05/11 18:03:37]

Done.

+
+  // If a scroll node exists, it should have an associated transform node.
+  DCHECK(transform_tree_index() != TransformTree::kInvalidNodeId);
+
+  PropertyTrees* property_trees = layer_tree_host_->property_trees();

[wkorman, 2017/05/11 17:42:13]

Use auto& like you did in other patch?

[pdr., 2017/05/11 18:03:37]

Done

+  property_trees->scroll_tree.SetScrollOffset(id(), scroll_offset);
+  TransformNode* 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);
+  return true;
+}
+
 void Layer::SetScrollClipLayerId(int clip_layer_id) {
   DCHECK(IsPropertyChangeAllowed());
   if (inputs_.scroll_clip_layer_id == clip_layer_id)