Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp

Index: third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp b/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
index 733cd67c26aeb10382b9a7bc3fe6202527124a5d..f9c8466ec466b6a03b445fc287c12122c285386e 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
+++ b/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
@@ -9,28 +9,27 @@
 
 namespace blink {
 
-FloatClipRect GeometryMapper::sourceToDestinationVisualRect(
+const FloatClipRect& GeometryMapper::sourceToDestinationVisualRect(
     const FloatRect& rect,
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState) {
   bool success = false;
-  FloatClipRect result = sourceToDestinationVisualRectInternal(
+  const FloatClipRect& result = sourceToDestinationVisualRectInternal(
       rect, sourceState, destinationState, success);
   DCHECK(success);
   return result;
 }
 
-FloatClipRect GeometryMapper::sourceToDestinationVisualRectInternal(
+const FloatClipRect& GeometryMapper::sourceToDestinationVisualRectInternal(
     const FloatRect& rect,
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState,
     bool& success) {
-  FloatClipRect result = localToAncestorVisualRectInternal(
+  const FloatClipRect& result = localToAncestorVisualRectInternal(
       rect, sourceState, destinationState, success);
   // Success if destinationState is an ancestor state.
   if (success)
     return result;
-

[Xianzhu, 2017/03/10 00:06:49]

Nit: Keep the blank line.

[chrishtr, 2017/03/10 01:45:29]

Done.

   // Otherwise first map to the lowest common ancestor, then map to destination.
   const TransformPaintPropertyNode* lcaTransform = lowestCommonAncestor(
       sourceState.transform(), destinationState.transform());
@@ -42,78 +41,86 @@ FloatClipRect GeometryMapper::sourceToDestinationVisualRectInternal(
   PropertyTreeState lcaState = destinationState;
   lcaState.setTransform(lcaTransform);
 
-  result =
+  const FloatClipRect& result2 =
       localToAncestorVisualRectInternal(rect, sourceState, lcaState, success);
   if (!success)
-    return result;
-  if (!result.isInfinite()) {
-    FloatRect final = ancestorToLocalRect(result.rect(), lcaTransform,
-                                          destinationState.transform());
-    result.setRect(final);
+    return result2;
+  if (!result2.isInfinite()) {
+    FloatRect rect = result2.rect();
+    ancestorToLocalRect(lcaTransform, destinationState.transform(), rect);
+    m_tempRect.setRect(rect);
+    if (result2.hasRadius())
+      m_tempRect.setHasRadius();
+    return m_tempRect;
   }
-  return result;
+  return result2;
 }
 
-FloatRect GeometryMapper::sourceToDestinationRect(
-    const FloatRect& rect,
+void GeometryMapper::sourceToDestinationRect(
     const TransformPaintPropertyNode* sourceTransformNode,
-    const TransformPaintPropertyNode* destinationTransformNode) {
+    const TransformPaintPropertyNode* destinationTransformNode,
+    FloatRect& rect) {
   bool success = false;
-  FloatRect result = localToAncestorRectInternal(
-      rect, sourceTransformNode, destinationTransformNode, success);
+  localToAncestorRectInternal(sourceTransformNode, destinationTransformNode,
+                              rect, success);
   // Success if destinationTransformNode is an ancestor of sourceTransformNode.
   if (success)
-    return result;
+    return;
 
   // Otherwise first map to the least common ancestor, then map to destination.
   const TransformPaintPropertyNode* lcaTransform =
       lowestCommonAncestor(sourceTransformNode, destinationTransformNode);
   DCHECK(lcaTransform);
 
-  FloatRect lcaRect =
-      localToAncestorRect(rect, sourceTransformNode, lcaTransform);
-  return ancestorToLocalRect(lcaRect, lcaTransform, destinationTransformNode);
+  localToAncestorRect(sourceTransformNode, lcaTransform, rect);
+  ancestorToLocalRect(lcaTransform, destinationTransformNode, rect);
 }
 
-FloatClipRect GeometryMapper::localToAncestorVisualRect(
+const FloatClipRect& GeometryMapper::localToAncestorVisualRect(
     const FloatRect& rect,
     const PropertyTreeState& localState,
     const PropertyTreeState& ancestorState) {
   bool success = false;
-  FloatClipRect result = localToAncestorVisualRectInternal(
+  const FloatClipRect& result = localToAncestorVisualRectInternal(
       rect, localState, ancestorState, success);
   DCHECK(success);
   return result;
 }
 
-FloatClipRect GeometryMapper::localToAncestorVisualRectInternal(
+const FloatClipRect& GeometryMapper::localToAncestorVisualRectInternal(

[Xianzhu, 2017/03/10 00:06:49]

It seems that we always return a temporary rect from this function, so returning
a reference seems not to worth the complexity.

Can we just use an output parameter?

[chrishtr, 2017/03/10 01:45:29]

Done. Good idea, it made the code a lot simpler, letting me remove a bunch of
hasRadius stuff since
visual rect use cases don't care about rounded corners.

     const FloatRect& rect,
     const PropertyTreeState& localState,
     const PropertyTreeState& ancestorState,
     bool& success) {
   if (localState == ancestorState) {
     success = true;
-    return rect;
+    m_tempRect = rect;
+    return m_tempRect;
   }
 
   if (localState.effect() != ancestorState.effect()) {
-    return slowLocalToAncestorVisualRectWithEffects(rect, localState,
-                                                    ancestorState, success);
+    m_tempRect = slowLocalToAncestorVisualRectWithEffects(
+        rect, localState, ancestorState, success);
+    return m_tempRect;
   }
 
   const auto& transformMatrix = localToAncestorMatrixInternal(
       localState.transform(), ancestorState.transform(), success);
-  if (!success)
-    return rect;
+  if (!success) {
+    m_tempRect = rect;
+    return m_tempRect;
+  }
 
   FloatRect mappedRect = transformMatrix.mapRect(rect);
 
-  FloatClipRect clipRect =
+  const FloatClipRect& clipRect =
       localToAncestorClipRectInternal(localState.clip(), ancestorState.clip(),
                                       ancestorState.transform(), success);
 
   if (success) {
-    clipRect.intersect(mappedRect);
+    m_tempRect = clipRect;
+    m_tempRect.intersect(mappedRect);
+    return m_tempRect;
   } else if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
     // On SPv1 we may fail when the paint invalidation container creates an
     // overflow clip (in ancestorState) which is not in localState of an
@@ -171,47 +178,46 @@ FloatClipRect GeometryMapper::slowLocalToAncestorVisualRectWithEffects(
   return result;
 }
 
-FloatRect GeometryMapper::localToAncestorRect(
-    const FloatRect& rect,
+void GeometryMapper::localToAncestorRect(
     const TransformPaintPropertyNode* localTransformNode,
-    const TransformPaintPropertyNode* ancestorTransformNode) {
+    const TransformPaintPropertyNode* ancestorTransformNode,
+    FloatRect& rect) {
   bool success = false;
-  FloatRect result = localToAncestorRectInternal(
-      rect, localTransformNode, ancestorTransformNode, success);
+  localToAncestorRectInternal(localTransformNode, ancestorTransformNode, rect,
+                              success);
   DCHECK(success);
-  return result;
 }
 
-FloatRect GeometryMapper::localToAncestorRectInternal(
-    const FloatRect& rect,
+void GeometryMapper::localToAncestorRectInternal(
     const TransformPaintPropertyNode* localTransformNode,
     const TransformPaintPropertyNode* ancestorTransformNode,
+    FloatRect& rect,
     bool& success) {
   if (localTransformNode == ancestorTransformNode) {
     success = true;
-    return rect;
+    return;
   }
 
   const auto& transformMatrix = localToAncestorMatrixInternal(
       localTransformNode, ancestorTransformNode, success);
   if (!success)
-    return rect;
-  return transformMatrix.mapRect(rect);
+    return;
+  rect = transformMatrix.mapRect(rect);
 }
 
-FloatRect GeometryMapper::ancestorToLocalRect(
-    const FloatRect& rect,
+void GeometryMapper::ancestorToLocalRect(
     const TransformPaintPropertyNode* ancestorTransformNode,
-    const TransformPaintPropertyNode* localTransformNode) {
+    const TransformPaintPropertyNode* localTransformNode,
+    FloatRect& rect) {
   if (localTransformNode == ancestorTransformNode)
-    return rect;
+    return;
 
   const auto& transformMatrix =
       localToAncestorMatrix(localTransformNode, ancestorTransformNode);
   DCHECK(transformMatrix.isInvertible());
 
   // TODO(chrishtr): Cache the inverse?
-  return transformMatrix.inverse().mapRect(rect);
+  rect = transformMatrix.inverse().mapRect(rect);
 }
 
 FloatClipRect GeometryMapper::localToAncestorClipRect(
@@ -227,22 +233,22 @@ FloatClipRect GeometryMapper::localToAncestorClipRect(
   return result;
 }
 
-FloatClipRect GeometryMapper::sourceToDestinationClipRect(
+const FloatClipRect& GeometryMapper::sourceToDestinationClipRect(
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState) {
   bool success = false;
-  FloatClipRect result = sourceToDestinationClipRectInternal(
+  const FloatClipRect& result = sourceToDestinationClipRectInternal(
       sourceState, destinationState, success);
   DCHECK(success);
 
   return result;
 }
 
-FloatClipRect GeometryMapper::sourceToDestinationClipRectInternal(
+const FloatClipRect& GeometryMapper::sourceToDestinationClipRectInternal(
     const PropertyTreeState& sourceState,
     const PropertyTreeState& destinationState,
     bool& success) {
-  FloatClipRect result = localToAncestorClipRectInternal(
+  const FloatClipRect& result = localToAncestorClipRectInternal(
       sourceState.clip(), destinationState.clip(), destinationState.transform(),
       success);
   // Success if destinationState is an ancestor state.
@@ -261,8 +267,8 @@ FloatClipRect GeometryMapper::sourceToDestinationClipRectInternal(
   PropertyTreeState lcaState = destinationState;
   lcaState.setTransform(lcaTransform);
 
-  result = localToAncestorClipRectInternal(sourceState.clip(), lcaState.clip(),
-                                           lcaState.transform(), success);
+  const FloatClipRect& result2 = localToAncestorClipRectInternal(
+      sourceState.clip(), lcaState.clip(), lcaState.transform(), success);
   if (!success) {
     if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
       // On SPv1 we may fail when the paint invalidation container creates an
@@ -273,14 +279,17 @@ FloatClipRect GeometryMapper::sourceToDestinationClipRectInternal(
       // Ignore it for SPv1 for now.
       success = true;
     }
-    return result;
+    return result2;
   }
-  if (!result.isInfinite()) {
-    FloatRect final = ancestorToLocalRect(result.rect(), lcaTransform,
-                                          destinationState.transform());
-    result.setRect(final);
+  if (!result2.isInfinite()) {
+    FloatRect rect = result2.rect();
+    ancestorToLocalRect(lcaTransform, destinationState.transform(), rect);
+    m_tempRect.setRect(rect);
+    if (result2.hasRadius())
+      m_tempRect.setHasRadius();
+    return m_tempRect;
   }
-  return result;
+  return result2;
 }
 
 const FloatClipRect& GeometryMapper::localToAncestorClipRectInternal(