Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator

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

 // Copyright 2016 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 "core/paint/PaintInvalidator.h"
 
 #include "core/editing/FrameSelection.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
 #include "core/layout/LayoutBlockFlow.h"
 #include "core/layout/LayoutObject.h"
 #include "core/layout/LayoutTable.h"
 #include "core/layout/LayoutView.h"
 #include "core/layout/svg/SVGLayoutSupport.h"
 #include "core/paint/ObjectPaintProperties.h"
 #include "core/paint/PaintLayer.h"
 #include "core/paint/PaintLayerScrollableArea.h"
 #include "core/paint/PaintPropertyTreeBuilder.h"
 
 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;
   }
 
   LayoutRect result(rect);

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

(Just curious, not needed for this patch)
Do these local result values cost anything? For example, for all of these
helpers, could we pass in a "LayoutRect&" and modify it in-place (like you did
in computeVisualRectInBacking)?

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

Tested and found that "LayoutRect&" is better than "const LayoutRect&" and
LayoutRect result in a standalone function, but has no difference in an inline
function.

   if (object.isLayoutView())
     toLayoutView(object).mapToVisualRectInAncestorSpace(
         &ancestor, result, InputIsInFrameCoordinates, DefaultVisualRectFlags);
   else
     object.mapToVisualRectInAncestorSpace(&ancestor, result);
   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()) {
       toLayoutBox(object).flipForWritingMode(rect);
     } else if (!(context.forcedSubtreeInvalidationFlags &
                  PaintInvalidatorContext::ForcedSubtreeSlowPathRect)) {
       // For SPv2 and the GeometryMapper path, we also need to convert the rect
       // for non-boxes into physical coordinates before applying paint offset.
       // (Otherwise we'll call mapToVisualrectInAncestorSpace() which requires
       // physical coordinates for boxes, but "physical coordinates with flipped
       // block-flow direction" for non-boxes for which we don't need to flip.)
       // TODO(wangxianzhu): Avoid containingBlock().
       object.containingBlock()->flipForWritingMode(rect);
     }
   }
 
   if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
     // In SPv2, visual rects are in the space of their local transform node.
     // 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.
     return LayoutRect(enclosingIntRect(rect));
   }
 
   LayoutRect result;
   if (context.forcedSubtreeInvalidationFlags &
       PaintInvalidatorContext::ForcedSubtreeSlowPathRect) {
     result = slowMapToVisualRectInAncestorSpace(
         object, *context.paintInvalidationContainer, rect);
   } else if (object == context.paintInvalidationContainer) {
     result = LayoutRect(rect);
   } else {
     // 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));
-    }
-
-    // Convert the result to the container's contents space.
-    result.moveBy(-context.paintInvalidationContainer->paintOffset());
+    result = mapLocalRectToPaintInvalidationContainerUsingPaintProperties(
+        geometryMapper, rect, context);
   }
 
   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);
 
   PaintLayer::mapRectInPaintInvalidationContainerToBacking(
       *context.paintInvalidationContainer, result);
 
   return result;
 }
 
 void PaintInvalidatorContext::mapLocalRectToPaintInvalidationBacking(
     const LayoutObject& object,
     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(
     const LayoutObject& object,
     const PaintInvalidatorContext& context) {
   // Use visual rect location for LayoutTexts because it suffices to check
   // visual rect change for layout caused invalidation.
   if (object.isText())
     return context.newVisualRect.location();
 
@@ skipping 284 matching lines @@
         PaintInvalidatorContext::ForcedSubtreeInvalidationRectUpdate;
 }
 
 void PaintInvalidator::processPendingDelayedPaintInvalidations() {
   for (auto target : m_pendingDelayedPaintInvalidations)
     target->getMutableForPainting().setShouldDoFullPaintInvalidation(
         PaintInvalidationDelayedFull);
 }
 
 }  // namespace blink