Get rid of LayerPainter::paintTransformedLayerIntoFragments().

Source/core/paint/LayerPainter.cpp

Index: Source/core/paint/LayerPainter.cpp
diff --git a/Source/core/paint/LayerPainter.cpp b/Source/core/paint/LayerPainter.cpp
index 7decbdab8ef7ead87009d1268579061095fb7688..d833a6221cffc607960afeeece814236f99d90a0 100644
--- a/Source/core/paint/LayerPainter.cpp
+++ b/Source/core/paint/LayerPainter.cpp
@@ -70,37 +70,7 @@ void LayerPainter::paintLayer(GraphicsContext* context, const LayerPaintingInfo&
 
     // PaintLayerAppliedTransform is used in RenderReplica, to avoid applying the transform twice.
     if (m_renderLayer.paintsWithTransform(paintingInfo.paintBehavior) && !(paintFlags & PaintLayerAppliedTransform)) {
-        TransformationMatrix layerTransform = m_renderLayer.renderableTransform(paintingInfo.paintBehavior);
-        // If the transform can't be inverted, then don't paint anything.
-        if (!layerTransform.isInvertible())
-            return;
-
-        if (m_renderLayer.enclosingPaginationLayer()) {
-            // FIXME: unify this one-off path with the code below.
-            paintTransformedLayerIntoFragments(context, paintingInfo, paintFlags);
-            return;
-        }
-
-        // Make sure the parent's clip rects have been calculated.
-        ClipRect clipRect = paintingInfo.paintDirtyRect;
-
-        OwnPtr<ClipRecorder> clipRecorder;
-        if (m_renderLayer.parent()) {
-            ClipRectsContext clipRectsContext(paintingInfo.rootLayer, (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize);
-            if (shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()) == IgnoreOverflowClip)
-                clipRectsContext.setIgnoreOverflowClip();
-            clipRect = m_renderLayer.clipper().backgroundClipRect(clipRectsContext);
-            clipRect.intersect(paintingInfo.paintDirtyRect);
-
-            if (needsToClip(paintingInfo, clipRect)) {
-                clipRecorder = adoptPtr(new ClipRecorder(m_renderLayer.parent()->renderer(), context, DisplayItem::ClipLayerParent, clipRect));
-                if (clipRect.hasRadius())
-                    applyRoundedRectClips(*m_renderLayer.parent(), paintingInfo, context, LayoutPoint(), paintFlags, *clipRecorder);
-            }
-        }
-
-        paintLayerByApplyingTransform(context, paintingInfo, paintFlags);
-
+        paintLayerWithTransform(context, paintingInfo, paintFlags);
         return;
     }
 
@@ -411,13 +381,70 @@ bool LayerPainter::atLeastOneFragmentIntersectsDamageRect(LayerFragments& fragme
     return false;
 }
 
-void LayerPainter::paintLayerByApplyingTransform(GraphicsContext* context, const LayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags, const LayoutPoint& translationOffset)
+void LayerPainter::paintLayerWithTransform(GraphicsContext* context, const LayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags)
+{
+    TransformationMatrix layerTransform = m_renderLayer.renderableTransform(paintingInfo.paintBehavior);
+    // If the transform can't be inverted, then don't paint anything.
+    if (!layerTransform.isInvertible())
+        return;
+
+    // If this is the "root" layer, clipping wrt. the ancestry has already been set up, which
+    // means that we can skip some unnecessary work (and even potentially incorrect work at
+    // that, if the ancestry has additional transforms).
+    RenderLayer* parentLayer = &m_renderLayer == paintingInfo.rootLayer ? 0 : m_renderLayer.parent();

[chrishtr, 2014/11/18 18:36:18]

This logic is new? Fixes a bug?

[mstensho (USE GERRIT), 2014/11/19 19:23:01]

Yes, it used to only check m_renderLayer.parent(). I found it ugly that we
risked jumping over paintingInfo.rootLayer that way, and walk all the way to
RenderView in search of the "root" layer (which we just skipped), so I changed
it. But I don't think it fixes anything, since
RenderLayerClipper::backgroundClipRect() catches the situation and returns an
infinite clip rect (with no radius). This means that needsToClip() will always
be false, so that we won't create a clip recorder (which would have used the
parent of the root layer if we actually got there -- scary).

So, thanks to something rather indistinguishable from luck, my change probably
doesn't fix anything at the moment, but I'd feel safer if we made sure to not
jump over the "root" layer and stumble all the way to the RenderView.

There's similar stuff all over the place in RenderLayerClipper, such as
backgroundClipRect(), which I pointed out above, but also getClipRects(),
calculateRects() and calculateClipRects(). They all make sure to stop at the
"root".

Everything should stop at the "root" layer. RenderLayer::convertToLayerCoords()
also does this, although there it's called ancestorLayer (a better name, IMHO).

So I prefer to do the same here, but if you prefer to keep it as it was, I'll
back it out.

The m_renderLayer.parent() check looked heavily misguided to me, but then I
found this, from 2008: http://trac.webkit.org/changeset/31007
Not sure if that means anything in current Blink, but in any case, my change
shouldn't make any difference in that regard.

[chrishtr, 2014/11/19 19:34:51]

I don't think it's good practice to change more than the minimum necessary
amount of code when factoring functions. It's hard enough as-is to verify
nothing was missed.

You can do that in a patch before or after instead.

[mstensho (USE GERRIT), 2014/11/19 22:39:06]

Done.

+
+    ClipRect clipRect(LayoutRect::infiniteRect());
+    if (parentLayer) {
+        // Calculate the clip rectangle that the ancestors establish.
+        ClipRectsContext clipRectsContext(paintingInfo.rootLayer, (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize);
+        if (shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()) == IgnoreOverflowClip)
+            clipRectsContext.setIgnoreOverflowClip();
+        clipRect = m_renderLayer.clipper().backgroundClipRect(clipRectsContext);
+    }
+
+    RenderLayer* paginationLayer = m_renderLayer.enclosingPaginationLayer();
+    LayerFragments fragments;
+    if (paginationLayer) {
+        ClipRectsCacheSlot cacheSlot = (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects;
+        ShouldRespectOverflowClip respectOverflowClip = shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer());
+        // Calculate the transformed bounding box in the current coordinate space, to figure out
+        // which fragmentainers (e.g. columns) we need to visit.
+        LayoutRect transformedExtent = RenderLayer::transparencyClipBox(&m_renderLayer, paginationLayer, RenderLayer::PaintingTransparencyClipBox, RenderLayer::RootOfTransparencyClipBox, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
+        paginationLayer->collectFragments(fragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, cacheSlot, IgnoreOverlayScrollbarSize, respectOverflowClip, 0, paintingInfo.subPixelAccumulation, &transformedExtent);

[chrishtr, 2014/11/18 18:36:18]

Add a RenderLayer::collectFragments method that hides the logic from lines
405-420.

Also, the existing RenderLayer::colectFragments collects all fragments below
paintingInfo.rootLayer, right? Why does this need to happen when painting layers
that are not the root, doesn't it cause redundant painting? Is this avoided by
having early returns in RenderLayer::colectFragments?

[mstensho (USE GERRIT), 2014/11/19 19:23:01]

OK, this isn't very pretty. Need to think a little more about it. This code also
potentially violates what I wrote in the review comment above regarding not
walking past the "root" layer.
It just collects the fragments for the layer passed. If you pass the pagination
layer (like here), it will collect all columns.
No redundant painting that I'm aware of.

[chrishtr, 2014/11/19 19:34:51]

Let's put in a big FIXME to look closely at this code and the code in
RenderLayer and fix any issues in it & refactor to make it obvious from code
structure what it does and that it's correct. Code as is is fine for this
refactor CL.

[mstensho (USE GERRIT), 2014/11/19 22:39:06]

Done.

+    } else {
+        // We don't need to collect any fragments in the regular way here. We have already
+        // calculated a clip rectangle for the ancestry if it was needed, and clipping this
+        // layer is something that can be done further down the path, when the transform has
+        // been applied.
+        fragments.append(LayerFragment());
+    }
+
+    for (const auto& fragment: fragments) {
+        ClipRect clipRectForFragment(clipRect);
+        if (paginationLayer) {
+            clipRectForFragment.moveBy(fragment.paginationOffset);
+            clipRectForFragment.intersect(fragment.backgroundRect);
+            if (clipRectForFragment.isEmpty())
+                continue;
+        }
+        OwnPtr<ClipRecorder> clipRecorder;
+        if (parentLayer && needsToClip(paintingInfo, clipRectForFragment)) {

[mstensho (USE GERRIT), 2014/11/19 19:23:01]

All the clipping stuff above for multicol should be conditioned on parentLayer.
Fixed that.

+            clipRecorder = adoptPtr(new ClipRecorder(parentLayer->renderer(), context, DisplayItem::ClipLayerParent, clipRectForFragment));
+            if (clipRectForFragment.hasRadius())

[chrishtr, 2014/11/18 18:36:18]

You're going to need to rebase on top of my latest patch.

[mstensho (USE GERRIT), 2014/11/19 19:23:01]

Done.

+                applyRoundedRectClips(*parentLayer, paintingInfo, context, fragment.paginationOffset, paintFlags, *clipRecorder);
+        }
+
+        paintFragmentByApplyingTransform(context, paintingInfo, paintFlags, fragment.paginationOffset);
+    }
+}
+
+void LayerPainter::paintFragmentByApplyingTransform(GraphicsContext* context, const LayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags, const LayoutPoint& fragmentTranslation)
 {
     // This involves subtracting out the position of the layer in our current coordinate space, but preserving
     // the accumulated error for sub-pixel layout.
     LayoutPoint delta;
     m_renderLayer.convertToLayerCoords(paintingInfo.rootLayer, delta);
-    delta.moveBy(translationOffset);
+    delta.moveBy(fragmentTranslation);
     TransformationMatrix transform(m_renderLayer.renderableTransform(paintingInfo.paintBehavior));
     IntPoint roundedDelta = roundedIntPoint(delta);
     transform.translateRight(roundedDelta.x(), roundedDelta.y());
@@ -770,40 +797,4 @@ void LayerPainter::paintOverlayScrollbars(GraphicsContext* context, const Layout
     m_renderLayer.setContainsDirtyOverlayScrollbars(false);
 }
 
-void LayerPainter::paintTransformedLayerIntoFragments(GraphicsContext* context, const LayerPaintingInfo& paintingInfo, PaintLayerFlags paintFlags)
-{
-    LayerFragments enclosingPaginationFragments;
-    LayoutPoint offsetOfPaginationLayerFromRoot;
-    LayoutRect transformedExtent = RenderLayer::transparencyClipBox(&m_renderLayer, m_renderLayer.enclosingPaginationLayer(), RenderLayer::PaintingTransparencyClipBox, RenderLayer::RootOfTransparencyClipBox, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
-    m_renderLayer.enclosingPaginationLayer()->collectFragments(enclosingPaginationFragments, paintingInfo.rootLayer, paintingInfo.paintDirtyRect,
-        (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize,
-        shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()), &offsetOfPaginationLayerFromRoot, paintingInfo.subPixelAccumulation, &transformedExtent);
-
-    for (size_t i = 0; i < enclosingPaginationFragments.size(); ++i) {
-        const LayerFragment& fragment = enclosingPaginationFragments.at(i);
-
-        // Apply the page/column clip for this fragment, as well as any clips established by layers in between us and
-        // the enclosing pagination layer.
-        LayoutRect clipRect = fragment.backgroundRect.rect();
-
-        // Now compute the clips within a given fragment
-        if (m_renderLayer.parent() != m_renderLayer.enclosingPaginationLayer()) {
-            m_renderLayer.enclosingPaginationLayer()->convertToLayerCoords(paintingInfo.rootLayer, offsetOfPaginationLayerFromRoot);
-
-            ClipRectsContext clipRectsContext(m_renderLayer.enclosingPaginationLayer(), (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize);
-            if (shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()) == IgnoreOverflowClip)
-                clipRectsContext.setIgnoreOverflowClip();
-            LayoutRect parentClipRect = m_renderLayer.clipper().backgroundClipRect(clipRectsContext).rect();
-            parentClipRect.moveBy(fragment.paginationOffset + offsetOfPaginationLayerFromRoot);
-            clipRect.intersect(parentClipRect);
-        }
-
-        OwnPtr<ClipRecorder> clipRecorder;
-        if (needsToClip(paintingInfo, clipRect))
-            clipRecorder = adoptPtr(new ClipRecorder(m_renderLayer.renderer(), context, DisplayItem::ClipLayerFragmentParent, clipRect));
-
-        paintLayerByApplyingTransform(context, paintingInfo, paintFlags, fragment.paginationOffset);
-    }
-}
-
 } // namespace blink