Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator

third_party/WebKit/Source/core/paint/PaintInvalidator.cpp

Index: third_party/WebKit/Source/core/paint/PaintInvalidator.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp b/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp
index 1aeca106f4747a34c6b96f6cab09c9375ebdb3b4..8b9d2f88e84afacfa1d5b39282b63e9f3c548a76 100644
--- a/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp
@@ -23,11 +23,11 @@ namespace blink {
 static LayoutRect slowMapToVisualRectInAncestorSpace(
     const LayoutObject& object,
     const LayoutBoxModelObject& ancestor,
-    const FloatRect& rect) {
+    const LayoutRect& rect) {
   if (object.isSVGChild()) {
     LayoutRect result;
-    SVGLayoutSupport::mapToVisualRectInAncestorSpace(object, &ancestor, rect,
-                                                     result);
+    SVGLayoutSupport::mapToVisualRectInAncestorSpace(object, &ancestor,
+                                                     FloatRect(rect), result);
     return result;
   }
 
@@ -40,21 +40,49 @@ static LayoutRect slowMapToVisualRectInAncestorSpace(
   return result;
 }
 
+template <typename Rect>
+static LayoutRect mapLocalRectToPaintInvalidationContainerUsingPaintProperties(

[pdr., 2017/01/18 02:31:18]

Nit: does "UsingPaintProperties" add information? I wonder if we could just use
"mapLocalRectToPaintInvalidationContainer".

+    GeometryMapper& geometryMapper,
+    const Rect& localRect,
+    const PaintInvalidatorContext& context) {
+  const auto* containerContentsProperties =
+      context.paintInvalidationContainer->paintProperties()
+          ->contentsProperties();
+  LayoutRect result;
+  if (context.treeBuilderContext.current.transform ==
+          containerContentsProperties->transform() &&
+      context.treeBuilderContext.current.clip ==
+          containerContentsProperties->clip()) {
+    result = LayoutRect(localRect);
+  } else {
+    PropertyTreeState currentTreeState(
+        context.treeBuilderContext.current.transform,
+        context.treeBuilderContext.current.clip, nullptr, nullptr);
+    result = LayoutRect(geometryMapper.sourceToDestinationVisualRect(
+        FloatRect(localRect), currentTreeState, *containerContentsProperties));
+  }
+
+  // Convert the result into the container's contents space.
+  result.moveBy(-context.paintInvalidationContainer->paintOffset());
+  return result;
+}
+
 // TODO(wangxianzhu): Combine this into
 // PaintInvalidator::mapLocalRectToBacking() when removing
 // PaintInvalidationState.
 static LayoutRect mapLocalRectToPaintInvalidationBacking(
     GeometryMapper& geometryMapper,
     const LayoutObject& object,
-    const FloatRect& localRect,
+    const LayoutRect& localRect,
     const PaintInvalidatorContext& context) {
+  // SVG children may use this path when mapping selection rects.
   bool isSVGChild = object.isSVGChild();
 
   // TODO(wkorman): The flip below is required because visual rects are
   // currently in "physical coordinates with flipped block-flow direction"
   // (see LayoutBoxModelObject.h) but we need them to be in physical
   // coordinates.
-  FloatRect rect = localRect;
+  LayoutRect rect = localRect;
   // Writing-mode flipping doesn't apply to non-root SVG.
   if (!isSVGChild) {
     if (object.isBox()) {
@@ -76,7 +104,7 @@ static LayoutRect mapLocalRectToPaintInvalidationBacking(
     // For SVG, the input rect is in local SVG coordinates in which paint
     // offset doesn't apply.
     if (!isSVGChild)
-      rect.moveBy(FloatPoint(object.paintOffset()));
+      rect.moveBy(object.paintOffset());
     // Use enclosingIntRect to ensure the final visual rect will cover the
     // rect in source coordinates no matter if the painting will use pixel
     // snapping.
@@ -94,31 +122,52 @@ static LayoutRect mapLocalRectToPaintInvalidationBacking(
     // For non-root SVG, the input rect is in local SVG coordinates in which
     // paint offset doesn't apply.
     if (!isSVGChild) {
-      rect.moveBy(FloatPoint(object.paintOffset()));
+      rect.moveBy(object.paintOffset());
       // Use enclosingIntRect to ensure the final visual rect will cover the
       // rect in source coordinates no matter if the painting will use pixel
       // snapping.
-      rect = enclosingIntRect(rect);
+      rect = LayoutRect(enclosingIntRect(rect));
     }
 
-    const auto* containerContentsProperties =
-        context.paintInvalidationContainer->paintProperties()
-            ->contentsProperties();
-    if (context.treeBuilderContext.current.transform ==
-            containerContentsProperties->transform() &&
-        context.treeBuilderContext.current.clip ==
-            containerContentsProperties->clip()) {
-      result = LayoutRect(rect);
-    } else {
-      PropertyTreeState currentTreeState(
-          context.treeBuilderContext.current.transform,
-          context.treeBuilderContext.current.clip, nullptr, nullptr);
-      result = LayoutRect(geometryMapper.sourceToDestinationVisualRect(
-          rect, currentTreeState, *containerContentsProperties));
-    }
+    result = mapLocalRectToPaintInvalidationContainerUsingPaintProperties(
+        geometryMapper, rect, context);
+  }
 
-    // Convert the result to the container's contents space.
-    result.moveBy(-context.paintInvalidationContainer->paintOffset());
+  object.adjustVisualRectForRasterEffects(result);
+
+  PaintLayer::mapRectInPaintInvalidationContainerToBacking(
+      *context.paintInvalidationContainer, result);
+  return result;
+}
+
+// TODO(wangxianzhu): Combine this into
+// PaintInvalidator::mapLocalRectToBacking() when removing
+// PaintInvalidationState.
+static LayoutRect mapSVGLocalRectToPaintInvalidationBacking(

[pdr., 2017/01/18 02:31:19]

I like the perf benefit here, but I don't like how this copies the SVG-specific
parts of mapLocalRectToPaintInvalidationBacking so we have to maintain the
parallel SVG codepath in mapLocalRectToPaintInvalidationBacking.

Would it be possible to templatize mapLocalRectToPaintInvalidationBacking like
you did for mapLocalRectToPaintInvalidationContainerUsingPaintProperties? If
not, does the SVG selection rect code need to be in LayoutRects?

[Xianzhu, 2017/01/18 06:23:02]

Good idea. Done.

+    GeometryMapper& geometryMapper,
+    const LayoutObject& object,
+    const FloatRect& localRect,
+    const PaintInvalidatorContext& context) {
+  DCHECK(object.isSVGChild());
+
+  if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+    // In SPv2, visual rects are in the space of their local transform node.
+    // Use enclosingIntRect to ensure the final visual rect will cover the
+    // rect in source coordinates no matter if the painting will use pixel
+    // snapping.
+    return LayoutRect(enclosingIntRect(localRect));
+  }
+
+  LayoutRect result;
+  if (context.forcedSubtreeInvalidationFlags &
+      PaintInvalidatorContext::ForcedSubtreeSlowPathRect) {
+    SVGLayoutSupport::mapToVisualRectInAncestorSpace(
+        object, context.paintInvalidationContainer, localRect, result);
+  } else if (object == context.paintInvalidationContainer) {
+    result = LayoutRect(localRect);
+  } else {
+    result = mapLocalRectToPaintInvalidationContainerUsingPaintProperties(
+        geometryMapper, localRect, context);
   }
 
   object.adjustVisualRectForRasterEffects(result);
@@ -134,27 +183,19 @@ void PaintInvalidatorContext::mapLocalRectToPaintInvalidationBacking(
     LayoutRect& rect) const {
   GeometryMapper geometryMapper;
   rect = blink::mapLocalRectToPaintInvalidationBacking(geometryMapper, object,
-                                                       FloatRect(rect), *this);
-}
-
-LayoutRect PaintInvalidator::mapLocalRectToPaintInvalidationBacking(
-    const LayoutObject& object,
-    const FloatRect& localRect,
-    const PaintInvalidatorContext& context) {
-  return blink::mapLocalRectToPaintInvalidationBacking(m_geometryMapper, object,
-                                                       localRect, context);
+                                                       rect, *this);
 }
 
 LayoutRect PaintInvalidator::computeVisualRectInBacking(
     const LayoutObject& object,
     const PaintInvalidatorContext& context) {
-  FloatRect localRect;
-  if (object.isSVGChild())
-    localRect = SVGLayoutSupport::localVisualRect(object);
-  else
-    localRect = FloatRect(object.localVisualRect());
-
-  return mapLocalRectToPaintInvalidationBacking(object, localRect, context);
+  if (object.isSVGChild()) {
+    return mapSVGLocalRectToPaintInvalidationBacking(
+        m_geometryMapper, object, SVGLayoutSupport::localVisualRect(object),
+        context);
+  }
+  return mapLocalRectToPaintInvalidationBacking(
+      m_geometryMapper, object, object.localVisualRect(), context);
 }
 
 LayoutPoint PaintInvalidator::computeLocationInBacking(