cc: Remove SetNeedsCommitNoRebuild from Layer API

cc/layers/layer.cc

Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index aad3951fad76014f7738db11c453a2fcdb4aa73a..c11d12a8e9a2ada672fc8b6602cec110449ff707 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -29,6 +29,7 @@
 #include "cc/trees/layer_tree_impl.h"
 #include "cc/trees/mutable_properties.h"
 #include "cc/trees/mutator_host.h"
+#include "cc/trees/scroll_node.h"
 #include "cc/trees/transform_node.h"
 #include "third_party/skia/include/core/SkImageFilter.h"
 #include "ui/gfx/geometry/rect_conversions.h"
@@ -91,6 +92,7 @@ Layer::Layer()
       force_render_surface_for_testing_(false),
       subtree_property_changed_(false),
       may_contain_video_(false),
+      is_scroll_clip_layer_(false),
       safe_opaque_background_color_(0),
       draw_blend_mode_(SkBlendMode::kSrcOver),
       num_unclipped_descendants_(0) {}
@@ -161,19 +163,6 @@ void Layer::SetNeedsCommit() {
     return;
 
   SetNeedsPushProperties();
-  layer_tree_host_->property_trees()->needs_rebuild = true;
-
-  if (ignore_set_needs_commit_)
-    return;
-
-  layer_tree_host_->SetNeedsCommit();
-}
-
-void Layer::SetNeedsCommitNoRebuild() {
-  if (!layer_tree_host_)
-    return;
-
-  SetNeedsPushProperties();
 
   if (ignore_set_needs_commit_)
     return;
@@ -306,8 +295,22 @@ void Layer::SetBounds(const gfx::Size& size) {
   if (!layer_tree_host_)
     return;
 
-  if (masks_to_bounds())
+  if (masks_to_bounds()) {
     SetSubtreePropertyChanged();
+    PropertyTreesNeedRebuild();
+  }
+
+  if (scrollable()) {
+    if (ScrollNode* node =
+            layer_tree_host_->property_trees()
+                ->scroll_tree.UpdateNodeFromOwningLayerId(id())) {
+      node->bounds = inputs_.bounds;
+    }
+  }
+
+  if (is_scroll_clip_layer_)
+    layer_tree_host_->property_trees()->scroll_tree.set_needs_update(true);
+
   SetNeedsCommit();
 }
 
@@ -361,6 +364,7 @@ void Layer::RequestCopyOfOutput(std::unique_ptr<CopyOutputRequest> request) {
     return;
   inputs_.copy_requests.push_back(std::move(request));
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -369,6 +373,7 @@ void Layer::SetBackgroundColor(SkColor background_color) {
   if (inputs_.background_color == background_color)
     return;
   inputs_.background_color = background_color;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -395,6 +400,7 @@ void Layer::SetMasksToBounds(bool masks_to_bounds) {
     return;
   inputs_.masks_to_bounds = masks_to_bounds;
   SetNeedsCommit();
+  PropertyTreesNeedRebuild();
   SetSubtreePropertyChanged();
 }
 
@@ -437,6 +443,7 @@ void Layer::SetFilters(const FilterOperations& filters) {
     inputs_.mask_layer->SetLayerMaskType(
         Layer::LayerMaskType::SINGLE_TEXTURE_MASK);
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -446,6 +453,7 @@ void Layer::SetBackgroundFilters(const FilterOperations& filters) {
     return;
   inputs_.background_filters = filters;
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -455,6 +463,7 @@ void Layer::SetFiltersOrigin(const gfx::PointF& filters_origin) {
     return;
   inputs_.filters_origin = filters_origin;
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -473,14 +482,14 @@ void Layer::SetOpacity(float opacity) {
   if (layer_tree_host_ && !force_rebuild) {
     PropertyTrees* property_trees = layer_tree_host_->property_trees();
     if (EffectNode* node =
-            property_trees->effect_tree.FindNodeFromOwningLayerId(id())) {
+            property_trees->effect_tree.UpdateNodeFromOwningLayerId(id())) {
       node->opacity = opacity;
       node->effect_changed = true;
       property_trees->effect_tree.set_needs_update(true);
-      SetNeedsCommitNoRebuild();
-      return;
     }
   }
+  if (layer_tree_host_ && force_rebuild)
+    layer_tree_host_->property_trees()->needs_rebuild = true;

[danakj, 2017/03/17 14:26:51]

Use the new method? Then you don't need to check if layer_tree_host_ here also
fwiw

[weiliangc, 2017/03/17 20:58:41]

Done.

   SetNeedsCommit();
 }
 
@@ -545,6 +554,7 @@ void Layer::SetBlendMode(SkBlendMode blend_mode) {
   inputs_.blend_mode = blend_mode;
   SetNeedsCommit();
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
 }
 
 void Layer::SetIsRootForIsolatedGroup(bool root) {
@@ -552,6 +562,7 @@ void Layer::SetIsRootForIsolatedGroup(bool root) {
   if (inputs_.is_root_for_isolated_group == root)
     return;
   inputs_.is_root_for_isolated_group = root;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -562,6 +573,7 @@ void Layer::SetContentsOpaque(bool opaque) {
   inputs_.contents_opaque = opaque;
   SetNeedsCommit();
   SetSubtreePropertyChanged();
+  PropertyTreesNeedRebuild();
 }
 
 void Layer::SetPosition(const gfx::PointF& position) {
@@ -576,7 +588,7 @@ void Layer::SetPosition(const gfx::PointF& position) {
   SetSubtreePropertyChanged();
   PropertyTrees* property_trees = layer_tree_host_->property_trees();
   if (TransformNode* transform_node =
-          property_trees->transform_tree.FindNodeFromOwningLayerId(id())) {
+          property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
     DCHECK_EQ(transform_tree_index(), transform_node->id);
     transform_node->update_post_local_transform(position, transform_origin());
     if (transform_node->sticky_position_constraint_id >= 0) {
@@ -591,8 +603,6 @@ void Layer::SetPosition(const gfx::PointF& position) {
     transform_node->needs_local_transform_update = true;
     transform_node->transform_changed = true;
     layer_tree_host_->property_trees()->transform_tree.set_needs_update(true);
-    SetNeedsCommitNoRebuild();
-    return;
   }
 
   SetNeedsCommit();
@@ -626,7 +636,7 @@ void Layer::SetTransform(const gfx::Transform& transform) {
   if (layer_tree_host_) {
     PropertyTrees* property_trees = layer_tree_host_->property_trees();
     if (TransformNode* transform_node =
-            property_trees->transform_tree.FindNodeFromOwningLayerId(id())) {
+            property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
       // We need to trigger a rebuild if we could have affected 2d axis
       // alignment. We'll check to see if transform and inputs_.transform are
       // axis align with respect to one another.
@@ -637,12 +647,8 @@ void Layer::SetTransform(const gfx::Transform& transform) {
       transform_node->needs_local_transform_update = true;
       transform_node->transform_changed = true;
       layer_tree_host_->property_trees()->transform_tree.set_needs_update(true);
-      if (preserves_2d_axis_alignment)
-        SetNeedsCommitNoRebuild();
-      else
-        SetNeedsCommit();
-      inputs_.transform = transform;
-      return;
+      if (!preserves_2d_axis_alignment)
+        layer_tree_host_->property_trees()->needs_rebuild = true;

[danakj, 2017/03/17 14:26:51]

Use new method?

[weiliangc, 2017/03/17 20:58:41]

Done.

     }
   }
 
@@ -663,15 +669,13 @@ void Layer::SetTransformOrigin(const gfx::Point3F& transform_origin) {
   SetSubtreePropertyChanged();
   PropertyTrees* property_trees = layer_tree_host_->property_trees();
   if (TransformNode* transform_node =
-          property_trees->transform_tree.FindNodeFromOwningLayerId(id())) {
+          property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
     DCHECK_EQ(transform_tree_index(), transform_node->id);
     transform_node->update_pre_local_transform(transform_origin);
     transform_node->update_post_local_transform(position(), transform_origin);
     transform_node->needs_local_transform_update = true;
     transform_node->transform_changed = true;
     layer_tree_host_->property_trees()->transform_tree.set_needs_update(true);
-    SetNeedsCommitNoRebuild();
-    return;
   }
 
   SetNeedsCommit();
@@ -699,6 +703,7 @@ void Layer::SetScrollParent(Layer* parent) {
   if (inputs_.scroll_parent)
     inputs_.scroll_parent->AddScrollChild(this);
 
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -729,6 +734,7 @@ void Layer::SetClipParent(Layer* ancestor) {
   if (inputs_.clip_parent)
     inputs_.clip_parent->AddClipChild(this);
 
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
   if (layer_tree_host_)
     layer_tree_host_->SetNeedsMetaInfoRecomputation(true);
@@ -763,13 +769,11 @@ void Layer::SetScrollOffset(const gfx::ScrollOffset& scroll_offset) {
     property_trees->scroll_tree.SetScrollOffset(id(), scroll_offset);
 
   if (TransformNode* transform_node =
-          property_trees->transform_tree.FindNodeFromOwningLayerId(id())) {
+          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);
-    SetNeedsCommitNoRebuild();
-    return;
   }
 
   SetNeedsCommit();
@@ -786,24 +790,18 @@ void Layer::SetScrollOffsetFromImplSide(
   inputs_.scroll_offset = scroll_offset;
   SetNeedsPushProperties();
 
-  bool needs_rebuild = true;
-
   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.FindNodeFromOwningLayerId(id())) {
+          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);
-    needs_rebuild = false;
   }
 
-  if (needs_rebuild)
-    property_trees->needs_rebuild = true;
-
   if (!inputs_.did_scroll_callback.is_null())
     inputs_.did_scroll_callback.Run(scroll_offset);
 
@@ -816,6 +814,7 @@ void Layer::SetScrollClipLayerId(int clip_layer_id) {
   if (inputs_.scroll_clip_layer_id == clip_layer_id)
     return;
   inputs_.scroll_clip_layer_id = clip_layer_id;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -844,6 +843,7 @@ void Layer::AddMainThreadScrollingReasons(
     return;
   inputs_.main_thread_scrolling_reasons = new_reasons;
   didUpdateMainThreadScrollingReasons();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -857,6 +857,7 @@ void Layer::ClearMainThreadScrollingReasons(
     return;
   inputs_.main_thread_scrolling_reasons = new_reasons;
   didUpdateMainThreadScrollingReasons();
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -865,6 +866,7 @@ void Layer::SetNonFastScrollableRegion(const Region& region) {
   if (inputs_.non_fast_scrollable_region == region)
     return;
   inputs_.non_fast_scrollable_region = region;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -874,6 +876,7 @@ void Layer::SetTouchEventHandlerRegion(const Region& region) {
     return;
 
   inputs_.touch_event_handler_region = region;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -882,6 +885,7 @@ void Layer::SetForceRenderSurfaceForTesting(bool force) {
   if (force_render_surface_for_testing_ == force)
     return;
   force_render_surface_for_testing_ = force;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -891,6 +895,7 @@ void Layer::SetDoubleSided(bool double_sided) {
     return;
   inputs_.double_sided = double_sided;
   SetNeedsCommit();
+  PropertyTreesNeedRebuild();
   SetSubtreePropertyChanged();
 }
 
@@ -900,6 +905,7 @@ void Layer::Set3dSortingContextId(int id) {
     return;
   inputs_.sorting_context_id = id;
   SetNeedsCommit();
+  PropertyTreesNeedRebuild();
   SetSubtreePropertyChanged();
 }
 
@@ -978,12 +984,18 @@ void Layer::InvalidatePropertyTreesIndices() {
   SetScrollTreeIndex(ScrollTree::kInvalidNodeId);
 }
 
+void Layer::PropertyTreesNeedRebuild() {
+  if (layer_tree_host_)
+    layer_tree_host_->property_trees()->needs_rebuild = true;
+}
+
 void Layer::SetShouldFlattenTransform(bool should_flatten) {
   DCHECK(IsPropertyChangeAllowed());
   if (inputs_.should_flatten_transform == should_flatten)
     return;
   inputs_.should_flatten_transform = should_flatten;
   SetNeedsCommit();
+  PropertyTreesNeedRebuild();
   SetSubtreePropertyChanged();
 }
 
@@ -1026,6 +1038,7 @@ void Layer::SetHideLayerAndSubtree(bool hide) {
 
   inputs_.hide_layer_and_subtree = hide;
   SetNeedsCommit();
+  PropertyTreesNeedRebuild();
   SetSubtreePropertyChanged();
 }
 
@@ -1058,8 +1071,10 @@ void Layer::SetIsContainerForFixedPositionLayers(bool container) {
     return;
 
   // Only request a commit if we have a fixed positioned descendant.
-  if (DescendantIsFixedToContainerLayer())
+  if (DescendantIsFixedToContainerLayer()) {
+    PropertyTreesNeedRebuild();
     SetNeedsCommit();
+  }
 }
 
 void Layer::SetPositionConstraint(const LayerPositionConstraint& constraint) {
@@ -1067,6 +1082,7 @@ void Layer::SetPositionConstraint(const LayerPositionConstraint& constraint) {
   if (inputs_.position_constraint == constraint)
     return;
   inputs_.position_constraint = constraint;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -1076,6 +1092,7 @@ void Layer::SetStickyPositionConstraint(
   if (inputs_.sticky_position_constraint == constraint)
     return;
   inputs_.sticky_position_constraint = constraint;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -1226,6 +1243,7 @@ void Layer::UpdateDrawsContent(bool has_drawable_content) {
     parent()->AddDrawableDescendants(draws_content ? 1 : -1);
 
   draws_content_ = draws_content;
+  PropertyTreesNeedRebuild();
   SetNeedsCommit();
 }
 
@@ -1327,19 +1345,17 @@ void Layer::OnIsAnimatingChanged(const PropertyAnimationState& mask,
   DCHECK(layer_tree_host_);
   PropertyTrees* property_trees = layer_tree_host_->property_trees();
 
-  TransformNode* transform_node =
-      property_trees->transform_tree.FindNodeFromOwningLayerId(id());
-  DCHECK(!transform_node || transform_node->id == transform_tree_index());
-
-  EffectNode* effect_node =
-      property_trees->effect_tree.FindNodeFromOwningLayerId(id());
-  DCHECK(!effect_node || effect_node->id == effect_tree_index());
-
   for (int property = TargetProperty::FIRST_TARGET_PROPERTY;
        property <= TargetProperty::LAST_TARGET_PROPERTY; ++property) {
     switch (property) {
       case TargetProperty::TRANSFORM:
-        if (transform_node) {
+        if (!mask.currently_running[property] &&
+            !mask.potentially_animating[property])
+          continue;

[ajuma, 2017/03/16 23:42:11]

This check can go outside the switch, then it doesn't need to be repeated for
each property.

[weiliangc, 2017/03/17 20:58:41]

Done.

+        if (TransformNode* transform_node =
+                property_trees->transform_tree.UpdateNodeFromOwningLayerId(
+                    id())) {
+          DCHECK_EQ(transform_node->id, transform_tree_index());
           if (mask.currently_running[property])
             transform_node->is_currently_animating =
                 state.currently_running[property];
@@ -1354,10 +1370,17 @@ void Layer::OnIsAnimatingChanged(const PropertyAnimationState& mask,
             }
             property_trees->transform_tree.set_needs_update(true);
           }
+        } else {
+          DCHECK(property_trees->needs_rebuild)
+              << "Attempting to animate non existent transform node";
         }
         break;
       case TargetProperty::OPACITY:
-        if (effect_node) {
+        if (!mask.currently_running[property] &&
+            !mask.potentially_animating[property])
+          continue;
+        if (EffectNode* effect_node =
+                property_trees->effect_tree.UpdateNodeFromOwningLayerId(id())) {
           if (mask.currently_running[property])
             effect_node->is_currently_animating_opacity =
                 state.currently_running[property];
@@ -1367,16 +1390,26 @@ void Layer::OnIsAnimatingChanged(const PropertyAnimationState& mask,
                 OpacityCanAnimateOnImplThread();
             property_trees->effect_tree.set_needs_update(true);
           }
+        } else {
+          DCHECK(property_trees->needs_rebuild)
+              << "Attempting to animate opacity on non existent effect node";
         }
         break;
       case TargetProperty::FILTER:
-        if (effect_node) {
+        if (!mask.currently_running[property] &&
+            !mask.potentially_animating[property])
+          continue;
+        if (EffectNode* effect_node =
+                property_trees->effect_tree.UpdateNodeFromOwningLayerId(id())) {
           if (mask.currently_running[property])
             effect_node->is_currently_animating_filter =
                 state.currently_running[property];
           if (mask.potentially_animating[property])
             effect_node->has_potential_filter_animation =
                 state.potentially_animating[property];
+        } else {
+          DCHECK(property_trees->needs_rebuild)
+              << "Attempting to animate filter on non existent effect node";
         }
         break;
       default: