cc: Compute sublayer scale from effect tree

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 54fbe3616932fe1617e476c292e1ae153f420beb..c86e40eb56da4578a2726135983c4073b0f9edb6 100644
--- a/cc/trees/draw_property_utils.cc
+++ b/cc/trees/draw_property_utils.cc
@@ -57,6 +57,7 @@ template <typename LayerType>
 bool ComputeClipRectInTargetSpace(const LayerType* layer,
                                   const ClipNode* clip_node,
                                   const TransformTree& transform_tree,
+                                  const EffectTree& effect_tree,
                                   int target_node_id,
                                   gfx::RectF* clip_rect_in_target_space) {
   DCHECK(layer->clip_tree_index() == clip_node->id);
@@ -67,8 +68,24 @@ bool ComputeClipRectInTargetSpace(const LayerType* layer,
     // 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
     // target.
-    if (transform_tree.ComputeTransformWithDestinationSublayerScale(
-            clip_node->data.target_id, target_node_id, &clip_to_target)) {
+    if (transform_tree.ComputeTransform(clip_node->data.target_id,
+                                        target_node_id, &clip_to_target)) {
+      if (target_node_id != 0) {

[ajuma, 2016/07/04 15:11:50]

Does node 0 need to be special-cased only because sublayer_layer is always (1,1)
there, or does something else go wrong in that case? Please add a comment.

[jaydasika, 2016/07/06 18:52:31]

Done.

+        const EffectNode* effect_node =
+            effect_tree.Node(layer->effect_tree_index());
+        const EffectNode* target_effect_node =
+            effect_node->data.has_render_surface
+                ? effect_node
+                : effect_tree.Node(effect_node->data.target_id);
+        const TransformNode* target_transform_node =
+            transform_tree.Node(target_node_id);

[ajuma, 2016/07/04 15:11:50]

Since this is used only for the DCHECK, it might be better to move this into the
DCHECK rather than creating a local variable for it.

[jaydasika, 2016/07/06 18:52:31]

Done.

+        DCHECK(target_transform_node->data.sublayer_scale ==

[ajuma, 2016/07/04 15:11:50]

Can this be a DCHECK_EQ instead?

[jaydasika, 2016/07/06 18:52:31]

DCHECK_EQ doesn't seem to support comparing vectors.

+               target_effect_node->data.sublayer_scale);
+        clip_to_target.matrix().postScale(
+            target_effect_node->data.sublayer_scale.x(),
+            target_effect_node->data.sublayer_scale.y(), 1.f);
+      }
+
       const TransformNode* source_node =
           transform_tree.Node(clip_node->data.target_id);
       if (source_node->data.sublayer_scale.x() != 0.f &&
@@ -254,7 +271,7 @@ void CalculateClipRects(
 
         // Compute the clip rect in target space and store it.
         if (!ComputeClipRectInTargetSpace(layer, clip_node, transform_tree,
-                                          target_node_id,
+                                          effect_tree, target_node_id,
                                           &clip_rect_in_target_space))
           continue;
       }
@@ -271,16 +288,18 @@ void CalculateClipRects(
 bool GetLayerClipRect(const scoped_refptr<Layer> layer,
                       const ClipNode* clip_node,
                       const TransformTree& transform_tree,
+                      const EffectTree& effect_tree,
                       int target_node_id,
                       gfx::RectF* clip_rect_in_target_space) {
   return ComputeClipRectInTargetSpace(layer.get(), clip_node, transform_tree,
-                                      target_node_id,
+                                      effect_tree, target_node_id,
                                       clip_rect_in_target_space);
 }
 
 bool GetLayerClipRect(const LayerImpl* layer,
                       const ClipNode* clip_node,
                       const TransformTree& transform_tree,
+                      const EffectTree& effect_tree,
                       int target_node_id,
                       gfx::RectF* clip_rect_in_target_space) {
   // This is equivalent of calling ComputeClipRectInTargetSpace.
@@ -389,7 +408,8 @@ void CalculateVisibleRects(
       // 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, transform_tree, target_node_id,
+      if (!GetLayerClipRect(layer, clip_node, transform_tree, effect_tree,
+                            target_node_id,
                             &combined_clip_rect_in_target_space)) {
         layer->set_visible_layer_rect(gfx::Rect(layer_bounds));
         continue;
@@ -903,17 +923,29 @@ static void VerifyDrawTransformsMatch(LayerImpl* layer,
   const int source_id = layer->transform_tree_index();
   int destination_id = FindTargetTransformTreeIndexFromEffectTree(
       property_trees->effect_tree, layer->effect_tree_index());
-  // TODO(jaydasika) : Remove this after sorting out how sublayer scale works
-  // for these ids.
-  if (destination_id == 0 || destination_id == 1)
-    return;
   gfx::Transform draw_transform;
   property_trees->transform_tree.ComputeTransform(source_id, destination_id,
                                                   &draw_transform);
-  TransformNode* target_node =
+  TransformNode* target_transform_node =
       property_trees->transform_tree.Node(destination_id);
-  draw_transform.matrix().postScale(target_node->data.sublayer_scale.x(),
-                                    target_node->data.sublayer_scale.y(), 1.f);
+  EffectNode* effect_node =
+      property_trees->effect_tree.Node(layer->effect_tree_index());
+  if (destination_id != 0) {

[ajuma, 2016/07/04 15:11:50]

Same comment as above about special-casing node 0.

[jaydasika, 2016/07/06 18:52:31]

Done.

+    int target_effect_id = effect_node->data.has_render_surface
+                               ? effect_node->id
+                               : effect_node->data.target_id;
+    EffectNode* target_effect_node =
+        property_trees->effect_tree.Node(target_effect_id);
+    DCHECK(target_transform_node->data.sublayer_scale ==

[ajuma, 2016/07/04 15:11:50]

DCHECK_EQ (and then we get the error message for free)

+           target_effect_node->data.sublayer_scale)
+        << " sublayer scale from transform tree: "
+        << target_transform_node->data.sublayer_scale.ToString()
+        << " sublayer scale from effect tree: "
+        << target_effect_node->data.sublayer_scale.ToString();
+    draw_transform.matrix().postScale(
+        target_effect_node->data.sublayer_scale.x(),
+        target_effect_node->data.sublayer_scale.y(), 1.f);
+  }
   if (layer->should_flatten_transform_from_property_tree())
     draw_transform.FlattenTo2d();
   draw_transform.Translate(layer->offset_to_transform_parent().x(),
@@ -1079,23 +1111,41 @@ gfx::Transform DrawTransform(const LayerImpl* layer,
   return xform;
 }
 
-static void SetSurfaceDrawTransform(const TransformTree& tree,
+static void SetSurfaceDrawTransform(const TransformTree& transform_tree,
+                                    const EffectTree& effect_tree,
                                     RenderSurfaceImpl* render_surface) {
-  const TransformNode* node = tree.Node(render_surface->TransformTreeIndex());
+  const TransformNode* transform_node =
+      transform_tree.Node(render_surface->TransformTreeIndex());
+  const EffectNode* effect_node =
+      effect_tree.Node(render_surface->EffectTreeIndex());
   // The draw transform of root render surface is identity tranform.
-  if (node->id == 1) {
+  if (transform_node->id == 1) {
     render_surface->SetDrawTransform(gfx::Transform());
     return;
   }
 
   gfx::Transform render_surface_transform;
-  const TransformNode* target_node = tree.Node(tree.TargetId(node->id));
-  tree.ComputeTransformWithDestinationSublayerScale(node->id, target_node->id,
-                                                    &render_surface_transform);
-  if (node->data.sublayer_scale.x() != 0.0 &&
-      node->data.sublayer_scale.y() != 0.0)
-    render_surface_transform.Scale(1.0 / node->data.sublayer_scale.x(),
-                                   1.0 / node->data.sublayer_scale.y());
+  const TransformNode* target_transform_node =
+      transform_tree.Node(transform_tree.TargetId(transform_node->id));
+  const EffectNode* target_effect_node =
+      effect_tree.Node(effect_node->data.target_id);
+  transform_tree.ComputeTransform(transform_node->id, target_transform_node->id,
+                                  &render_surface_transform);
+  if (target_transform_node->id != 0) {
+    DCHECK(target_transform_node->data.sublayer_scale ==

[ajuma, 2016/07/04 15:11:50]

Here too, same comments about node 0 and DCHECK_EQ :)

+           target_effect_node->data.sublayer_scale)
+        << target_transform_node->id << target_effect_node->id;
+    render_surface_transform.matrix().postScale(
+        target_effect_node->data.sublayer_scale.x(),
+        target_effect_node->data.sublayer_scale.y(), 1.f);
+  }

[ajuma, 2016/07/04 15:11:50]

Would it make sense to write a helper function with this logic and re-use it in
the other places above where we're computing and using sublayer scale?

[jaydasika, 2016/07/06 18:52:31]

Done.

+
+  DCHECK(transform_node->data.sublayer_scale ==
+         effect_node->data.sublayer_scale);
+  if (effect_node->data.sublayer_scale.x() != 0.0 &&
+      effect_node->data.sublayer_scale.y() != 0.0)
+    render_surface_transform.Scale(1.0 / effect_node->data.sublayer_scale.x(),
+                                   1.0 / effect_node->data.sublayer_scale.y());
   render_surface->SetDrawTransform(render_surface_transform);
 }
 
@@ -1289,7 +1339,8 @@ void ComputeSurfaceDrawProperties(const PropertyTrees* property_trees,
 
   SetSurfaceIsClipped(clip_node, render_surface);
   SetSurfaceDrawOpacity(property_trees->effect_tree, render_surface);
-  SetSurfaceDrawTransform(property_trees->transform_tree, render_surface);
+  SetSurfaceDrawTransform(property_trees->transform_tree,
+                          property_trees->effect_tree, render_surface);
   render_surface->SetScreenSpaceTransform(
       property_trees->transform_tree.ToScreenSpaceTransformWithoutSublayerScale(
           render_surface->TransformTreeIndex()));