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

cc/layers/layer.cc

 // Copyright 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/layers/layer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 758 matching lines @@
 void Layer::SetScrollOffset(const gfx::ScrollOffset& scroll_offset) {
   DCHECK(IsPropertyChangeAllowed());
 
   if (inputs_.scroll_offset == scroll_offset)
     return;
   inputs_.scroll_offset = 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();
 }
 
 void Layer::SetScrollOffsetFromImplSide(
     const gfx::ScrollOffset& scroll_offset) {
   DCHECK(IsPropertyChangeAllowed());
   // This function only gets called during a BeginMainFrame, so there
   // is no need to call SetNeedsUpdate here.
   DCHECK(layer_tree_host_ && layer_tree_host_->CommitRequested());
   if (inputs_.scroll_offset == scroll_offset)
     return;
   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())
     inputs_.did_scroll_callback.Run(scroll_offset);
 
   // The callback could potentially change the layer structure:
   // "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)
     return;
   inputs_.scroll_clip_layer_id = clip_layer_id;
   SetPropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
 Layer* Layer::scroll_clip_layer() const {
@@ skipping 607 matching lines @@
       ->subtree_has_copy_request;
 }
 
 gfx::Transform Layer::ScreenSpaceTransform() const {
   DCHECK_NE(transform_tree_index_, TransformTree::kInvalidNodeId);
   return draw_property_utils::ScreenSpaceTransform(
       this, layer_tree_host_->property_trees()->transform_tree);
 }
 
 }  // namespace cc