Changes subtreeShouldRenderToSeparateSurface logic to account for explicit clipping

cc/layer_tree_host_common.cc

Index: cc/layer_tree_host_common.cc
diff --git a/cc/layer_tree_host_common.cc b/cc/layer_tree_host_common.cc
index 0a204ceb6914df96a5bdede2b6c8c9ee7e1c6915..81b9aa68b7efbc275f3ef0b78e18c9dca7aa9587 100644
--- a/cc/layer_tree_host_common.cc
+++ b/cc/layer_tree_host_common.cc
@@ -274,7 +274,7 @@ static bool subtreeShouldRenderToSeparateSurface(LayerType* layer, bool axisAlig
         return true;
 
     // If the layer clips its descendants but it is not axis-aligned with respect to its parent.
-    if (layerClipsSubtree(layer) && !axisAlignedWithRespectToParent && numDescendantsThatDrawContent > 0)
+    if (layerClipsSubtree(layer) && !layer->drawProperties().all_children_can_clip)
         return true;
 
     // If the layer has some translucency and does not have a preserves-3d transform style.
@@ -429,21 +429,46 @@ static inline void removeSurfaceForEarlyExit(LayerType* layerToRemove, LayerList
     layerToRemove->clearRenderSurface();
 }
 
+inline bool isPositiveScaleOrTranslation(const gfx::Transform& xform)
+{
+    if (!xform.IsScaleOrTranslation())
+        return false;
+
+    // Grab our scale and make sure it's positive.
+    float x_scale = xform.matrix().getDouble(0,0);

[jamesr, 2012/12/14 04:31:19]

do we have to do a double->float conversion here?  I suspect it'll compile away,
but since the transform is double and we don't need to do anything but a sign
comparison seems better to keep this doubles

[whunt, 2012/12/14 18:17:01]

There's no reason to do the conversion.  I'm just used to assuming things are
floats.  I'll chnage it.

On 2012/12/14 04:31:19, jamesr wrote:

+    float y_scale = xform.matrix().getDouble(1,1);
+    if (x_scale <= 0.0f || y_scale <= 0.0f)
+        return false;
+
+    return true;
+}
+
 // Recursively walks the layer tree to compute any information that is needed
 // before doing the main recursion.
 template<typename LayerType>
 static void preCalculateMetaInformation(LayerType* layer)
 {
     int numDescendantsThatDrawContent = 0;
+    bool allChildrenCanClip = true;
+
+    bool sublayerPreventsClip = !isPositiveScaleOrTranslation(layer->sublayerTransform());

[jamesr, 2012/12/14 04:31:19]

I'm not sure that this is sufficient.  If the transform on the clipping layer is
a (for instance) rotation then we can clip a texutre_layer_impl, but we don't
have any code (afaik) to clip other types of layers

[shawnsingh, 2012/12/14 07:34:44]

+1 james comment; we do need the additional condition that prevents
sublayerPreventsClip from being true unless the layer is the correct type,
right?

also, naming nit:  sublayerPreventsClip --> descendantPreventsClip

[Ian Vollick, 2012/12/14 12:02:16]

Sorry for the drive-by, but I wanted to mention that we should probably add
SkMatrix44::preservesAxisAlignment to match the already existing SkMatrix::
preservesAxisAlignment. I think this would suit your needs here. Implementing
the SkMatrix44 method should be pretty simple, I think -- correct me if I'm
wrong, but if in the upper-left 3x3 matrix there are only 3 non-zero entries,
one in each row, and one in each column, I think we're ok?

[whunt, 2012/12/14 18:17:01]

I'm aware this isn't sufficient.  I'm working on fixing it.  I just wanted to
get the ball rolling.

On 2012/12/14 04:31:19, jamesr wrote:

[whunt, 2012/12/14 18:17:01]

The clipper wont work on rotations by 90 degrees so the functionality isn't the
same.

On 2012/12/14 12:02:16, vollick wrote:

 
     for (size_t i = 0; i < layer->children().size(); ++i) {
         LayerType* childLayer = layer->children()[i];
         preCalculateMetaInformation<LayerType>(childLayer);
-        numDescendantsThatDrawContent += childLayer->drawsContent() ? 1 : 0;
-        numDescendantsThatDrawContent += childLayer->drawProperties().num_descendants_that_draw_content;
+
+        int numChildDescendantsThatDrawContent = 0;
+        numChildDescendantsThatDrawContent += childLayer->drawsContent() ? 1 : 0;

[shawnsingh, 2012/12/14 07:34:44]

I'm not sure about this rename, how about we omit this particular change from
this patch and discuss it later?  no functionality changed here, right?

[whunt, 2012/12/14 18:17:01]

I need the intermediate value.  I didn't change your variables name.  Look at
the code more closely.

On 2012/12/14 07:34:44, shawnsingh wrote:

+        numChildDescendantsThatDrawContent += childLayer->drawProperties().num_descendants_that_draw_content;
+        numDescendantsThatDrawContent += numChildDescendantsThatDrawContent;
+
+        if (!childLayer->drawProperties().all_children_can_clip ||
+            numChildDescendantsThatDrawContent > 0 && (sublayerPreventsClip || !isPositiveScaleOrTranslation(childLayer->transform())))
+            allChildrenCanClip = false;
     }
 
     layer->drawProperties().num_descendants_that_draw_content = numDescendantsThatDrawContent;
+    layer->drawProperties().all_children_can_clip = allChildrenCanClip;
 }
 
 // Recursively walks the layer tree starting at the given node and computes all the