Account for filter bounds for backgroundRect during PaintLayer painting

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

Index: third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp b/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp
index ac670bf730d2374730a42870f98319e2bd4f9368..912f843fe87fcb44fef3f7a9e186016127fa3850 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp
@@ -202,7 +202,8 @@ LayoutRect PaintLayerClipper::localClipRect(
   if (m_geometryMapper) {
     ClipRect clipRect;
     calculateClipRectWithGeometryMapper(context, false, clipRect);
-    applyOverflowClipToBackgroundRectWithGeometryMapper(context, clipRect);
+    applyVisualOverflowClipToBackgroundRectWithGeometryMapper(context,
+                                                              clipRect);

[trchen, 2017/03/29 23:50:08]

Why is this line needed? The only caller of PLC::localClipRect() is
PaintLayer::boundingBoxForCompositingInternal(), and the caller intersect the
rect with physicalBoundingBox() immediately.

[chrishtr, 2017/03/30 04:35:26]

There is not really any point, but if I remove this line then the
GeometryMapper mode of this class will disagree with non-GeometryMapper,
which delegates to calculateRects. I don't think it does any harm.

     LayoutRect premappedRect = clipRect.rect();
 
     // The rect now needs to be transformed to the local space of this
@@ -298,8 +299,8 @@ void PaintLayerClipper::calculateRectsWithGeometryMapper(
     backgroundRect.move(context.subPixelAccumulation);
     backgroundRect.intersect(paintDirtyRect);
 
-    applyOverflowClipToBackgroundRectWithGeometryMapper(context,
-                                                        backgroundRect);
+    applyVisualOverflowClipToBackgroundRectWithGeometryMapper(context,
+                                                              backgroundRect);
 
     calculateClipRectWithGeometryMapper(context, true, foregroundRect);
     foregroundRect.move(context.subPixelAccumulation);
@@ -379,6 +380,10 @@ void PaintLayerClipper::calculateRects(
     // PaintLayer are in physical coordinates, so the overflow has to be
     // flipped.
     toLayoutBox(layoutObject).flipForWritingMode(layerBoundsWithVisualOverflow);
+    if (m_layer.paintsWithFilters()) {
+      layerBoundsWithVisualOverflow =
+          m_layer.mapLayoutRectForFilter(layerBoundsWithVisualOverflow);
+    }
     layerBoundsWithVisualOverflow.moveBy(offset);
     backgroundRect.intersect(layerBoundsWithVisualOverflow);
   }
@@ -492,9 +497,10 @@ void PaintLayerClipper::calculateClipRectWithGeometryMapper(
   output.moveBy(-context.rootLayer->layoutObject().paintOffset());
 }
 
-void PaintLayerClipper::applyOverflowClipToBackgroundRectWithGeometryMapper(
-    const ClipRectsContext& context,
-    ClipRect& clip) const {
+void PaintLayerClipper::
+    applyVisualOverflowClipToBackgroundRectWithGeometryMapper(
+        const ClipRectsContext& context,
+        ClipRect& clip) const {

[trchen, 2017/03/29 23:50:08]

The name doesn't seem to match the implementation... (In fact the implementation
looks weird.)

It seems to me that clip would be a in-out parameter, and the expected input
would be the accumulated clip that should apply to all phases of the current
layer. Then if the current object does not create its own overflow clip, returns
the accumulated clip untouched. Otherwise, return its propagating visual
overflow clipped by the accumulated clip.

[chrishtr, 2017/03/30 04:35:26]

Method is now deleted.

   const LayoutObject& layoutObject = m_layer.layoutObject();
   if (!shouldClipOverflow(context))
     return;
@@ -507,6 +513,11 @@ void PaintLayerClipper::applyOverflowClipToBackgroundRectWithGeometryMapper(
           // PaintLayer are in physical coordinates, so the overflow has to be
           // flipped.
           layerBoundsWithVisualOverflow);
+  if (m_layer.paintsWithFilters()) {
+    layerBoundsWithVisualOverflow =
+        m_layer.mapLayoutRectForFilter(layerBoundsWithVisualOverflow);
+  }
+

[trchen, 2017/03/29 23:50:08]

Is it possible to make it a geometry mapper helper? This line together with line
521 basically reads
geometryMapper->mapVisualRectToAncestor(rootLayerState, layerState,
layerBoundsWithVisualOverflow)
to me.

[chrishtr, 2017/03/30 04:35:26]

That's a great observation. In fact it led me to realize that this whole
method is not necessary, since I can call sourceToDestinationVisualRect
in cases when shouldClipOverflow() is true. This will be a significant
time savings in paint.

   mapLocalToRootWithGeometryMapper(context, layerBoundsWithVisualOverflow);
   clip.intersect(layerBoundsWithVisualOverflow);
 }