cc: Set clip rect to empty rect when layer's is_clipped is false

cc/trees/draw_property_utils.cc

Index: cc/trees/draw_property_utils.cc
diff --git a/cc/trees/draw_property_utils.cc b/cc/trees/draw_property_utils.cc
index 8894590fa3749eb505b84a14d908c4e45363a1ef..f6a5ab1ff728a3dddd4e7cb903ecb8652c711c45 100644
--- a/cc/trees/draw_property_utils.cc
+++ b/cc/trees/draw_property_utils.cc
@@ -274,49 +274,50 @@ void CalculateClipRects(const std::vector<LayerImpl*>& visible_layer_list,
   const ClipTree& clip_tree = property_trees->clip_tree;
   for (auto& layer : visible_layer_list) {
     const ClipNode* clip_node = clip_tree.Node(layer->clip_tree_index());
+    bool layer_needs_clip_rect =
+        non_root_surfaces_enabled
+            ? clip_node->layers_are_clipped
+            : clip_node->layers_are_clipped_when_surfaces_disabled;
+    if (!layer_needs_clip_rect) {
+      layer->set_clip_rect(gfx::Rect());
+      continue;
+    }
     if (!non_root_surfaces_enabled) {
       layer->set_clip_rect(
           gfx::ToEnclosingRect(clip_node->clip_in_target_space));
       continue;
     }
 
-    // When both the layer and the target are unclipped, the entire layer
-    // content rect is visible.
-    const bool fully_visible =
-        !clip_node->layers_are_clipped && !clip_node->target_is_clipped;

[weiliangc, 2016/11/02 21:08:07]

Could you explain why target_is_clipped is ok to be removed here?

[jaydasika, 2016/11/02 21:21:56]

if !non_root_surfaces_enabled = true, we wont reach here(line 288 in new code)

When non_root_surface_enabled,
if clip_node->layers_are_clipped = false, we wont reach here (line 283 in new
code).

So, here clip_node->layers_are_clipped = true always => fully_visible = false
always (irrespective of what target_is_clipped is)  

[weiliangc, 2016/11/02 21:34:28]

For !non_root_surface_enabled, old cold will set the root layer to
clip_in_target_space, and new code would set it to empty. This is expected?

For non_root_surface_enabled, and layers_are_clipped false, new code will set
clip rect to empty, and old code would set to empty or calculate the clip rect
depends on whether target_is_clipped? For target_is_clipped true case, does it
always ends being empty clip?

[jaydasika, 2016/11/02 21:46:40]

When layer is not clipped, clip rect is never used, so it doesn't matter what
its set to

[weiliangc, 2016/11/03 19:21:54]

Ah I see because !non_root_surface_enabled's root layer is not clipped, and
non_root_surface_enabled we use layers_are_clipped as signal to whether layer is
clipped.

Thanks for patient replies. :)

+    const TransformTree& transform_tree = property_trees->transform_tree;
+    const TransformNode* transform_node =
+        transform_tree.Node(layer->transform_tree_index());
+    int target_node_id = transform_tree.ContentTargetId(transform_node->id);
 
-    if (!fully_visible) {
-      const TransformTree& transform_tree = property_trees->transform_tree;
-      const TransformNode* transform_node =
-          transform_tree.Node(layer->transform_tree_index());
-      int target_node_id = transform_tree.ContentTargetId(transform_node->id);
-
-      // The clip node stores clip rect in its target space.
-      gfx::RectF clip_rect_in_target_space = clip_node->clip_in_target_space;
-
-      // If required, this clip rect should be mapped to the current layer's
-      // target space.
-      if (clip_node->target_transform_id != target_node_id) {
-        // In this case, layer has a clip parent or scroll parent (or shares the
-        // target with an ancestor layer that has clip parent) and the clip
-        // parent's target is different from the layer's target. As the layer's
-        // target has unclippped descendants, it is unclippped.
-        if (!clip_node->layers_are_clipped)
-          continue;
-
-        // Compute the clip rect in target space and store it.
-        bool for_visible_rect_calculation = false;
-        if (!ComputeClipRectInTargetSpace(
-                layer, clip_node, property_trees, target_node_id,
-                for_visible_rect_calculation, &clip_rect_in_target_space))
-          continue;
-      }
+    // The clip node stores clip rect in its target space.
+    gfx::RectF clip_rect_in_target_space = clip_node->clip_in_target_space;
 
-      if (!clip_rect_in_target_space.IsEmpty()) {
-        layer->set_clip_rect(gfx::ToEnclosingRect(clip_rect_in_target_space));
-      } else {
-        layer->set_clip_rect(gfx::Rect());
-      }
+    // If required, this clip rect should be mapped to the current layer's
+    // target space.
+    if (clip_node->target_transform_id != target_node_id) {
+      // In this case, layer has a clip parent or scroll parent (or shares the
+      // target with an ancestor layer that has clip parent) and the clip
+      // parent's target is different from the layer's target. As the layer's
+      // target has unclippped descendants, it is unclippped.
+      if (!clip_node->layers_are_clipped)
+        continue;
+
+      // Compute the clip rect in target space and store it.
+      bool for_visible_rect_calculation = false;
+      if (!ComputeClipRectInTargetSpace(
+              layer, clip_node, property_trees, target_node_id,
+              for_visible_rect_calculation, &clip_rect_in_target_space))
+        continue;
+    }
+
+    if (!clip_rect_in_target_space.IsEmpty()) {
+      layer->set_clip_rect(gfx::ToEnclosingRect(clip_rect_in_target_space));
+    } else {
+      layer->set_clip_rect(gfx::Rect());
     }
   }
 }
@@ -956,6 +957,7 @@ static void ComputeLayerClipRect(const PropertyTrees* property_trees,
                                  const LayerImpl* layer) {
   const EffectTree* effect_tree = &property_trees->effect_tree;
   const ClipTree* clip_tree = &property_trees->clip_tree;
+  const ClipNode* clip_node = clip_tree->Node(layer->clip_tree_index());
   const EffectNode* effect_node = effect_tree->Node(layer->effect_tree_index());
   const EffectNode* target_node =
       effect_node->has_render_surface
@@ -972,21 +974,20 @@ static void ComputeLayerClipRect(const PropertyTrees* property_trees,
       ComputeAccumulatedClip(property_trees, include_viewport_clip,
                              layer->clip_tree_index(), target_node->id);
 
+  bool is_clipped_from_clip_tree =
+      property_trees->non_root_surfaces_enabled
+          ? clip_node->layers_are_clipped
+          : clip_node->layers_are_clipped_when_surfaces_disabled;
+  DCHECK_EQ(is_clipped_from_clip_tree, accumulated_clip_rect.is_clipped);
+
   gfx::RectF accumulated_clip = accumulated_clip_rect.clip_rect;
 
-  if ((!property_trees->non_root_surfaces_enabled &&
-       clip_tree->Node(layer->clip_tree_index())
-           ->layers_are_clipped_when_surfaces_disabled) ||
-      clip_tree->Node(layer->clip_tree_index())->layers_are_clipped) {
-    DCHECK(layer->clip_rect() == gfx::ToEnclosingRect(accumulated_clip))
-        << " layer: " << layer->id() << " clip id: " << layer->clip_tree_index()
-        << " layer clip: " << layer->clip_rect().ToString() << " v.s. "
-        << gfx::ToEnclosingRect(accumulated_clip).ToString()
-        << " and clip node clip: "
-        << gfx::ToEnclosingRect(
-               clip_tree->Node(layer->clip_tree_index())->clip_in_target_space)
-               .ToString();
-  }
+  DCHECK(layer->clip_rect() == gfx::ToEnclosingRect(accumulated_clip))
+      << " layer: " << layer->id() << " clip id: " << layer->clip_tree_index()
+      << " layer clip: " << layer->clip_rect().ToString() << " v.s. "
+      << gfx::ToEnclosingRect(accumulated_clip).ToString()
+      << " and clip node clip: "
+      << gfx::ToEnclosingRect(clip_node->clip_in_target_space).ToString();
 }
 
 static void ComputeVisibleRectsInternal(