Changes subtreeShouldRenderToSeparateSurface logic to account for explicit clipping

cc/layer_tree_host_common.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/layer_tree_host_common.h"
 
 #include <algorithm>
 
 #include "base/debug/trace_event.h"
 #include "cc/layer.h"
@@ skipping 256 matching lines @@
         return true;
 
     int numDescendantsThatDrawContent = layer->drawProperties().num_descendants_that_draw_content;
 
     // If the layer flattens its subtree (i.e. the layer doesn't preserve-3d), but it is
     // treated as a 3D object by its parent (i.e. parent does preserve-3d).
     if (layerIsInExisting3DRenderingContext(layer) && !layer->preserves3D() && numDescendantsThatDrawContent > 0)
         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) && !axisAlignedWithRespectToParent && !layer->drawProperties().can_clip_self)
         return true;
 
     // If the layer has some translucency and does not have a preserves-3d transform style.
     // This condition only needs a render surface if two or more layers in the
     // subtree overlap. But checking layer overlaps is unnecessarily costly so
     // instead we conservatively create a surface whenever at least two layers
     // draw content for this subtree.
     bool atLeastTwoLayersInSubtreeDrawContent = layer->hasDelegatedContent() ||
         (numDescendantsThatDrawContent > 0 && (layer->drawsContent() || numDescendantsThatDrawContent > 1));
 
@@ skipping 134 matching lines @@
     // layers from the end of the list.
     while (renderSurfaceLayerList.back() != layerToRemove) {
         renderSurfaceLayerList.back()->clearRenderSurface();
         renderSurfaceLayerList.pop_back();
     }
     DCHECK(renderSurfaceLayerList.back() == layerToRemove);
     renderSurfaceLayerList.pop_back();
     layerToRemove->clearRenderSurface();
 }
 
+inline bool isPositiveScaleOrTranslation(const gfx::Transform& xform)

[shawnsingh, 2012/12/14 23:59:11]

Re: James' comment; I think a good place for this, for now, is MathUtil

[danakj, 2012/12/15 00:02:17]

nit: call the variable "transform"? we don't do this elsewhere.

+{
+    if (!xform.IsScaleOrTranslation())
+        return false;
+
+    // Grab our scale and make sure it's positive.
+    double x_scale = xform.matrix().getDouble(0, 0);
+    double y_scale = xform.matrix().getDouble(1, 1);

[danakj, 2012/12/15 00:02:17]

What about z_scale?

+    if (x_scale <= 0.0 || y_scale <= 0.0)
+        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 canClipSelf = true;
+    bool sublayerXformPreventsClip = !isPositiveScaleOrTranslation(layer->sublayerTransform());

[danakj, 2012/12/15 00:02:17]

nit: "sublayerTransform..."

 
     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;
+
+        if (childLayer->drawsContent() && !childLayer->CanClipSelf() ||

[jamesr, 2012/12/14 22:51:16]

multiple invocations of && and || merit some grouping parenthesis - compilers
can follow precedence rules correctly, but it's harder for humans to do so
quickly

+            !childLayer->drawProperties().can_clip_self ||
+            sublayerXformPreventsClip ||
+            !isPositiveScaleOrTranslation(childLayer->transform()))
+            canClipSelf = false;

[jamesr, 2012/12/14 22:51:16]

i would probably use braces around this block since it's a bit hard to tell the
end of the if and the start of the actual block

[shawnsingh, 2012/12/14 23:59:11]

looking at this if-statement, there is a triple-meaning to "can clip self" that
needs to be disambiguated.
(1) The layer's accessor "CanClipSelf" perhaps should say
"supportsSelfClipping".
(2) The drawProperties can_clip_self actually seems like it should say
"all_children_can_clip_themselves"
(3) the actual "layer can clip itself" canClipSelf is implicit in this
if-statement. I wonder if it would help readability to make that boolean
explicit, just so the variable name exists to help people see the flow of logic.

for example, something like this in code:

bool thisLayerCanClipItself = <opposite of stuff currently inside your if
statement>
if (!thisLayerCanClipItself)
   allChildrenCanClipThemselves = false;

     }
 
     layer->drawProperties().num_descendants_that_draw_content = numDescendantsThatDrawContent;
+    layer->drawProperties().can_clip_self = canClipSelf;
 }
 
 // Recursively walks the layer tree starting at the given node and computes all the
 // necessary transformations, clipRects, render surfaces, etc.
 template<typename LayerType, typename LayerList, typename RenderSurfaceType>
 static void calculateDrawPropertiesInternal(LayerType* layer, const gfx::Transform& parentMatrix,
     const gfx::Transform& fullHierarchyMatrix, const gfx::Transform& currentScrollCompensationMatrix,
     const gfx::Rect& clipRectFromAncestor, const gfx::Rect& clipRectFromAncestorInDescendantSpace, bool ancestorClipsSubtree,
     RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
     LayerSorter* layerSorter, int maxTextureSize, float deviceScaleFactor, float pageScaleFactor, bool subtreeCanUseLCDText,
@@ skipping 638 matching lines @@
 
   // At this point, we think the point does hit the touch event handler region on the layer, but we need to walk up
   // the parents to ensure that the layer was not clipped in such a way that the
   // hit point actually should not hit the layer.
   if (pointIsClippedBySurfaceOrClipRect(screenSpacePoint, layerImpl))
      return false;
 
   return true;
 }
 }  // namespace cc