cc: Include viewport clip in visible rect of clip children

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 7db6814424ebe5496040584f5d4ffc3e34553a1f..e320b4d319deaa183139766038e907cfdbf11cbb 100644
--- a/cc/trees/draw_property_utils.cc
+++ b/cc/trees/draw_property_utils.cc
@@ -89,7 +89,8 @@ bool ComputeClipRectInTargetSpace(const LayerImpl* layer,
                                   const ClipNode* clip_node,
                                   const PropertyTrees* property_trees,
                                   int target_node_id,
-                                  gfx::RectF* clip_rect_in_target_space) {
+                                  gfx::RectF* clip_rect_in_target_space,
+                                  bool use_combined_clip) {

[weiliangc, 2016/09/13 15:44:14]

clip_rect_in_target_space is output of this function. We should put output as
the last parameter, and use_combined_clip goes before that.

[jaydasika, 2016/09/13 17:17:56]

Done.

   DCHECK(layer->clip_tree_index() == clip_node->id);
   DCHECK(clip_node->target_transform_id != target_node_id);
 
@@ -97,6 +98,10 @@ bool ComputeClipRectInTargetSpace(const LayerImpl* layer,
   const EffectNode* target_effect_node =
       ContentsTargetEffectNode(layer->effect_tree_index(), effect_tree);
   gfx::Transform clip_to_target;
+  gfx::RectF clip_from_clip_node =
+      use_combined_clip ? clip_node->combined_clip_in_target_space
+                        : clip_node->clip_in_target_space;
+
   if (clip_node->target_transform_id > target_node_id) {
     // In this case, layer has a scroll parent. We need to keep the scale
     // at the layer's target but remove the scale at the scroll parent's
@@ -124,16 +129,16 @@ bool ComputeClipRectInTargetSpace(const LayerImpl* layer,
                                        clip_node->target_transform_id,
                                        effect_tree, transform_tree);
 #endif
-      *clip_rect_in_target_space = MathUtil::MapClippedRect(
-          clip_to_target, clip_node->clip_in_target_space);
+      *clip_rect_in_target_space =
+          MathUtil::MapClippedRect(clip_to_target, clip_from_clip_node);
     } else {
       return false;
     }
   } else {
     if (property_trees->ComputeTransformFromTarget(
             target_node_id, clip_node->target_effect_id, &clip_to_target)) {
-      *clip_rect_in_target_space = MathUtil::ProjectClippedRect(
-          clip_to_target, clip_node->clip_in_target_space);
+      *clip_rect_in_target_space =
+          MathUtil::ProjectClippedRect(clip_to_target, clip_from_clip_node);
     } else {
       return false;
     }
@@ -334,10 +339,13 @@ void CalculateClipRects(
         if (!clip_node->layers_are_clipped)
           continue;
 
-        // Compute the clip rect in target space and store it.
-        if (!ComputeClipRectInTargetSpace(layer, clip_node, property_trees,
-                                          target_node_id,
-                                          &clip_rect_in_target_space))
+        // Compute the clip rect in target space and store it. We don't use the
+        // combined clip for calculating clip rects as we don't want to reapply
+        // clips.
+        bool use_combined_clip = false;

[weiliangc, 2016/09/13 15:44:14]

nit: I think this name might not be easy to understand for people who are not
familiar with the code. How about renaming it to for_visible_rect_calculation or
something like that, since it is used for saying whether we are using the helper
function to calculate visible rect or clip rect. And inside the
ComputeClipRectInTargetSpace function, we can say for visible rect calculation
we use combined_clip?

[jaydasika, 2016/09/13 17:17:56]

Done.

+        if (!ComputeClipRectInTargetSpace(
+                layer, clip_node, property_trees, target_node_id,
+                &clip_rect_in_target_space, use_combined_clip))
           continue;
       }
 
@@ -350,17 +358,6 @@ void CalculateClipRects(
   }
 }
 
-bool GetLayerClipRect(const LayerImpl* layer,
-                      const ClipNode* clip_node,
-                      const PropertyTrees* property_trees,
-                      int target_node_id,
-                      gfx::RectF* clip_rect_in_target_space) {
-  // This is equivalent of calling ComputeClipRectInTargetSpace.
-  *clip_rect_in_target_space = gfx::RectF(layer->clip_rect());
-  return property_trees->transform_tree.Node(target_node_id)
-      ->ancestors_are_invertible;
-}
-
 void CalculateVisibleRects(const LayerImplList& visible_layer_list,
                            const PropertyTrees* property_trees,
                            bool non_root_surfaces_enabled) {
@@ -456,14 +453,10 @@ void CalculateVisibleRects(const LayerImplList& visible_layer_list,
         continue;
       }
 
-      // We use the clip node's clip_in_target_space (and not
-      // combined_clip_in_target_space) here because we want to clip
-      // with respect to clip parent's local clip and not its combined clip as
-      // the combined clip has even the clip parent's target's clip baked into
-      // it and as our target is different, we don't want to use it in our
-      // visible rect computation.
-      if (!GetLayerClipRect(layer, clip_node, property_trees, target_node_id,
-                            &combined_clip_rect_in_target_space)) {
+      bool use_combined_clip = true;
+      if (!ComputeClipRectInTargetSpace(
+              layer, clip_node, property_trees, target_node_id,
+              &combined_clip_rect_in_target_space, use_combined_clip)) {
         layer->set_visible_layer_rect(gfx::Rect(layer_bounds));
         continue;
       }