Get rid of the one remaining extra call to LayerPainter::beginTransparencyLayers.

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 ca4cdba201ab4e5cfce3007d8e410c5c1fe3a3e5..5eff16940344142d3fb548d2f40dd70b0c311e30 100644
--- a/Source/core/paint/LayerPainter.cpp
+++ b/Source/core/paint/LayerPainter.cpp
@@ -75,17 +75,8 @@ void LayerPainter::paintLayer(GraphicsContext* context, const LayerPaintingInfo&
         if (!layerTransform.isInvertible())
             return;
 
-        // If we have a transparency layer enclosing us and we are the root of a transform, then we need to establish the transparency
-        // layer from the parent now, assuming there is a parent
-        if (paintFlags & PaintLayerHaveTransparency) {
-            // FIXME: can we get rid of this? It would mean transparency starts after clipping in these cases.
-            if (m_renderLayer.parent())
-                LayerPainter(*m_renderLayer.parent()).beginTransparencyLayers(context, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

I think this change is actually what causes the invisible (to the naked eye)
rendering difference in fast/reflections/reflection-masks-opacity.html . We get
here when rendering the reflection (because RenderReplica uses transforms to do
its job). The RenderReplica is a child of what it replicates, i.e. the
RenderImage in this case. So we end up beginning transparency here for the
image.

Before your changes:
Set image transparency (0.5) {
    Set replica transparency (1.0) {
        Paint reflection.
    }
    Paint image.
}

After your changes:
Set replica transparency (1.0) {
    Set image transparency (0.5) {
        Paint reflection.
    }
}
Set image transparency (0.5) {
    Paint image.
}

So the order has changed. It used to be 0.5 followed by 1.0, but now it's 1.0
followed by 0.5. Not that it should matter in theory, but maybe it does in
practice.

Yeah, I think it's fine to just rebaseline for that test.

[chrishtr, 2014/11/14 20:11:37]

Done.

-            else
-                beginTransparencyLayers(context, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
-        }
-
         if (m_renderLayer.enclosingPaginationLayer()) {
+            // FIXME: unify this one-off path with the code below.

[mstensho (USE GERRIT), 2014/11/14 12:20:52]

I'm on it. :)

[chrishtr, 2014/11/14 20:11:37]

ack!

             paintTransformedLayerIntoFragments(context, paintingInfo, paintFlags);
             return;
         }
@@ -116,24 +107,58 @@ void LayerPainter::paintLayer(GraphicsContext* context, const LayerPaintingInfo&
     paintLayerContentsAndReflection(context, paintingInfo, paintFlags);
 }
 
-bool LayerPainter::shouldCreateTransparencyLayerForBlendMode()
-{
-    return !m_renderLayer.renderer()->isDocumentElement() && m_renderLayer.stackingNode()->isStackingContext() && m_renderLayer.hasNonIsolatedDescendantWithBlendMode();
-}
-
-void LayerPainter::beginTransparencyLayers(GraphicsContext* context, const RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize& subPixelAccumulation, PaintBehavior paintBehavior)
+bool shouldUseTransparencyLayer(RenderLayer& renderLayer, PaintBehavior paintBehavior)

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

static? Static inline, even?

[chrishtr, 2014/11/14 20:11:36]

Inlined.

 {
     // Blending operations must be performed only with the nearest ancestor stacking context.
     // Note that there is no need to create a transparency layer if we're painting the root.
     // FIXME: this should be unified further into RenderLayer::paintsWithTransparency().
-    if (!m_renderLayer.paintsWithTransparency(paintBehavior) && !shouldCreateTransparencyLayerForBlendMode())
-        return;
+    bool shouldUseTransparencyLayerForBlendMode = !renderLayer.renderer()->isDocumentElement() && renderLayer.stackingNode()->isStackingContext() && renderLayer.hasNonIsolatedDescendantWithBlendMode();
+    return renderLayer.paintsWithTransparency(paintBehavior) || shouldUseTransparencyLayerForBlendMode;

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

Might as well flip the expressions here. No need to call
renderLayer.paintsWithTransparency(paintBehavior) if you already know that
shouldUseTransparencyLayerForBlendMode is true. Then again, the common return
value from shouldUseTransparencyLayer() is false, so you'll end up having to
evaluate both in most cases anyway.

[chrishtr, 2014/11/14 20:11:36]

Done.

+}
 
-    // FIXME: refactor to remove this.
-    if (m_renderLayer.usedTransparency())
+class TransparencyLayerHelper {
+public:
+    TransparencyLayerHelper(GraphicsContext* context, RenderLayer& renderLayer, const RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize& subPixelAccumulation, PaintBehavior paintBehavior)
+        : m_transparencyLayerInProgress(false)
+        , m_context(context)
+    {
+        if (!shouldUseTransparencyLayer(renderLayer, paintBehavior))
+            return;
+
+        context->save();
+        LayoutRect clipRect = renderLayer.paintingExtent(rootLayer, paintDirtyRect, subPixelAccumulation, paintBehavior);
+        context->clip(clipRect);
+
+        if (renderLayer.renderer()->hasBlendMode())
+            context->setCompositeOperation(context->compositeOperation(), renderLayer.renderer()->style()->blendMode());
+
+        context->beginTransparencyLayer(renderLayer.renderer()->opacity());
+
+        if (renderLayer.renderer()->hasBlendMode())
+            context->setCompositeOperation(context->compositeOperation(), WebBlendModeNormal);
+#ifdef REVEAL_TRANSPARENCY_LAYERS
+        context->fillRect(clipRect, Color(0.0f, 0.0f, 0.5f, 0.2f));
+#endif
+        m_transparencyLayerInProgress = true;
+    }
+
+    ~TransparencyLayerHelper()
+    {
+        if (m_transparencyLayerInProgress) {

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

Couldn't you just NULL m_context if there's no transparency, so that we won't
need this flag? Ugly?

[chrishtr, 2014/11/14 20:11:36]

Feels ugly to me.

[mstensho (USE GERRIT), 2014/11/14 23:03:21]

Acknowledged.

+            m_context->endLayer();
+            m_context->restore();
+        }
+    }
+private:
+    bool m_transparencyLayerInProgress;
+    GraphicsContext* m_context;
+};
+
+void LayerPainter::beginTransparencyLayers(GraphicsContext* context, const RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, const LayoutSize& subPixelAccumulation, PaintBehavior paintBehavior)

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

Bye-bye to this then. :)

[chrishtr, 2014/11/14 20:11:36]

Yeah, forgot to delete. Sorry for the sloppy work here, I was too eager to send
this late last night. :)

[mstensho (USE GERRIT), 2014/11/14 23:03:21]

No worries. Didn't keep me from seeing the inner beauty of the patch. :)

+{
+    if (!shouldUseTransparencyLayer(m_renderLayer, paintBehavior))
         return;
 
-    m_renderLayer.setUsedTransparency(true);
     context->save();
     LayoutRect clipRect = m_renderLayer.paintingExtent(rootLayer, paintDirtyRect, subPixelAccumulation, paintBehavior);
     context->clip(clipRect);
@@ -272,88 +297,81 @@ void LayerPainter::paintLayerContents(GraphicsContext* context, const LayerPaint
 
     ClipPathHelper clipPathHelper(context, m_renderLayer, paintingInfo, rootRelativeBounds, rootRelativeBoundsComputed, offsetFromRoot, paintFlags);
 
-    beginTransparencyLayers(context, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
-
-    LayerPaintingInfo localPaintingInfo(paintingInfo);
-
-    LayerFragments layerFragments;
-    if (shouldPaintContent || shouldPaintOutline || isPaintingOverlayScrollbars) {
-        // Collect the fragments. This will compute the clip rectangles and paint offsets for each layer fragment.
-        m_renderLayer.collectFragments(layerFragments, localPaintingInfo.rootLayer, localPaintingInfo.paintDirtyRect,
-            (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize,
-            shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()), &offsetFromRoot, localPaintingInfo.subPixelAccumulation);
-        if (shouldPaintContent)
-            shouldPaintContent = atLeastOneFragmentIntersectsDamageRect(layerFragments, localPaintingInfo, paintFlags, offsetFromRoot);
-    }
-
-    bool selectionOnly = localPaintingInfo.paintBehavior & PaintBehaviorSelectionOnly;
-    // If this layer's renderer is a child of the paintingRoot, we render unconditionally, which
-    // is done by passing a nil paintingRoot down to our renderer (as if no paintingRoot was ever set).
-    // Else, our renderer tree may or may not contain the painting root, so we pass that root along
-    // so it will be tested against as we descend through the renderers.
-    RenderObject* paintingRootForRenderer = 0;
-    if (localPaintingInfo.paintingRoot && !m_renderLayer.renderer()->isDescendantOf(localPaintingInfo.paintingRoot))
-        paintingRootForRenderer = localPaintingInfo.paintingRoot;
-
-    { // Begin block for the lifetime of any filter.
-        FilterPainter filterPainter(m_renderLayer, context, offsetFromRoot, layerFragments.isEmpty() ? ClipRect() : layerFragments[0].backgroundRect, localPaintingInfo, paintFlags,
-            rootRelativeBounds, rootRelativeBoundsComputed);
-
-        ASSERT(!(localPaintingInfo.paintBehavior & PaintBehaviorForceBlackText));
-
-        bool shouldPaintBackground = isPaintingCompositedBackground && shouldPaintContent && !selectionOnly;
-        bool shouldPaintNegZOrderList = (isPaintingScrollingContent && isPaintingOverflowContents) || (!isPaintingScrollingContent && isPaintingCompositedBackground);
-        bool shouldPaintOwnContents = isPaintingCompositedForeground && shouldPaintContent;
-        bool shouldPaintNormalFlowAndPosZOrderLists = isPaintingCompositedForeground;
-        bool shouldPaintOverlayScrollbars = isPaintingOverlayScrollbars;
-
-        PaintBehavior paintBehavior = PaintBehaviorNormal;
-        if (paintFlags & PaintLayerPaintingSkipRootBackground)
-            paintBehavior |= PaintBehaviorSkipRootBackground;
-        else if (paintFlags & PaintLayerPaintingRootBackgroundOnly)
-            paintBehavior |= PaintBehaviorRootBackgroundOnly;
-
-        if (shouldPaintBackground) {
-            paintBackgroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
-                localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
+    {

[mstensho (USE GERRIT), 2014/11/14 12:20:51]

No need for this scope.

[chrishtr, 2014/11/14 20:11:36]

The reason I have a scope is to distinguish it from the lifetime of the
ClipPathHelper. Otherwise I'd be relying on whatever the order of destruction is
for stack-allocated variables.

[mstensho (USE GERRIT), 2014/11/14 23:03:22]

Aren't the objects guaranteed to be destroyed in reverse declaration order?

[chrishtr, 2014/11/14 23:05:45]

Probably, but I couldn't say that for sure without checking the spec. I think
it's worth it to make it obvious.

[mstensho (USE GERRIT), 2014/11/14 23:24:13]

https://isocpp.org/wiki/faq/dtors#order-dtors-for-locals (anything else would be
insane anyway)

I find those extra scopes ugly, which is why I'm pushing so hard here. :)

+        TransparencyLayerHelper transparencyLayerHelper(context, m_renderLayer, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
+
+        LayerPaintingInfo localPaintingInfo(paintingInfo);
+
+        LayerFragments layerFragments;
+        if (shouldPaintContent || shouldPaintOutline || isPaintingOverlayScrollbars) {
+            // Collect the fragments. This will compute the clip rectangles and paint offsets for each layer fragment.
+            m_renderLayer.collectFragments(layerFragments, localPaintingInfo.rootLayer, localPaintingInfo.paintDirtyRect,
+                (paintFlags & PaintLayerUncachedClipRects) ? UncachedClipRects : PaintingClipRects, IgnoreOverlayScrollbarSize,
+                shouldRespectOverflowClip(paintFlags, m_renderLayer.renderer()), &offsetFromRoot, localPaintingInfo.subPixelAccumulation);
+            if (shouldPaintContent)
+                shouldPaintContent = atLeastOneFragmentIntersectsDamageRect(layerFragments, localPaintingInfo, paintFlags, offsetFromRoot);
         }
 
-        if (shouldPaintNegZOrderList)
-            paintChildren(NegativeZOrderChildren, context, paintingInfo, paintFlags);
-
-        if (shouldPaintOwnContents) {
-            paintForegroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
-                localPaintingInfo, paintBehavior, paintingRootForRenderer, selectionOnly, paintFlags);
-        }
+        bool selectionOnly = localPaintingInfo.paintBehavior & PaintBehaviorSelectionOnly;
+        // If this layer's renderer is a child of the paintingRoot, we render unconditionally, which
+        // is done by passing a nil paintingRoot down to our renderer (as if no paintingRoot was ever set).
+        // Else, our renderer tree may or may not contain the painting root, so we pass that root along
+        // so it will be tested against as we descend through the renderers.
+        RenderObject* paintingRootForRenderer = 0;
+        if (localPaintingInfo.paintingRoot && !m_renderLayer.renderer()->isDescendantOf(localPaintingInfo.paintingRoot))
+            paintingRootForRenderer = localPaintingInfo.paintingRoot;
+
+        { // Begin block for the lifetime of any filter.
+            FilterPainter filterPainter(m_renderLayer, context, offsetFromRoot, layerFragments.isEmpty() ? ClipRect() : layerFragments[0].backgroundRect, localPaintingInfo, paintFlags,
+                rootRelativeBounds, rootRelativeBoundsComputed);
+
+            ASSERT(!(localPaintingInfo.paintBehavior & PaintBehaviorForceBlackText));
+
+            bool shouldPaintBackground = isPaintingCompositedBackground && shouldPaintContent && !selectionOnly;
+            bool shouldPaintNegZOrderList = (isPaintingScrollingContent && isPaintingOverflowContents) || (!isPaintingScrollingContent && isPaintingCompositedBackground);
+            bool shouldPaintOwnContents = isPaintingCompositedForeground && shouldPaintContent;
+            bool shouldPaintNormalFlowAndPosZOrderLists = isPaintingCompositedForeground;
+            bool shouldPaintOverlayScrollbars = isPaintingOverlayScrollbars;
+
+            PaintBehavior paintBehavior = PaintBehaviorNormal;
+            if (paintFlags & PaintLayerPaintingSkipRootBackground)
+                paintBehavior |= PaintBehaviorSkipRootBackground;
+            else if (paintFlags & PaintLayerPaintingRootBackgroundOnly)
+                paintBehavior |= PaintBehaviorRootBackgroundOnly;
+
+            if (shouldPaintBackground) {
+                paintBackgroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
+                    localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
+            }
 
-        if (shouldPaintOutline)
-            paintOutlineForFragments(layerFragments, context, localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
+            if (shouldPaintNegZOrderList)
+                paintChildren(NegativeZOrderChildren, context, paintingInfo, paintFlags);
 
-        if (shouldPaintNormalFlowAndPosZOrderLists)
-            paintChildren(NormalFlowChildren | PositiveZOrderChildren, context, paintingInfo, paintFlags);
+            if (shouldPaintOwnContents) {
+                paintForegroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
+                    localPaintingInfo, paintBehavior, paintingRootForRenderer, selectionOnly, paintFlags);
+            }
 
-        if (shouldPaintOverlayScrollbars)
-            paintOverflowControlsForFragments(layerFragments, context, localPaintingInfo, paintFlags);
-    } // Filter painter block
+            if (shouldPaintOutline)
+                paintOutlineForFragments(layerFragments, context, localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
 
-    bool shouldPaintMask = (paintFlags & PaintLayerPaintingCompositingMaskPhase) && shouldPaintContent && m_renderLayer.renderer()->hasMask() && !selectionOnly;
-    bool shouldPaintClippingMask = (paintFlags & PaintLayerPaintingChildClippingMaskPhase) && shouldPaintContent && !selectionOnly;
+            if (shouldPaintNormalFlowAndPosZOrderLists)
+                paintChildren(NormalFlowChildren | PositiveZOrderChildren, context, paintingInfo, paintFlags);
 
-    if (shouldPaintMask)
-        paintMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
+            if (shouldPaintOverlayScrollbars)
+                paintOverflowControlsForFragments(layerFragments, context, localPaintingInfo, paintFlags);
+        } // FilterPainter block
 
-    if (shouldPaintClippingMask) {
-        // Paint the border radius mask for the fragments.
-        paintChildClippingMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
-    }
+        bool shouldPaintMask = (paintFlags & PaintLayerPaintingCompositingMaskPhase) && shouldPaintContent && m_renderLayer.renderer()->hasMask() && !selectionOnly;
+        bool shouldPaintClippingMask = (paintFlags & PaintLayerPaintingChildClippingMaskPhase) && shouldPaintContent && !selectionOnly;
 
-    // End our transparency layer
-    if (((paintFlags & PaintLayerHaveTransparency) || shouldCreateTransparencyLayerForBlendMode()) && m_renderLayer.usedTransparency()
-        && !(m_renderLayer.reflectionInfo() && m_renderLayer.reflectionInfo()->isPaintingInsideReflection())) {
-        context->endLayer();
-        context->restore();
-        m_renderLayer.setUsedTransparency(false);
-    }
+        if (shouldPaintMask)
+            paintMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
+        if (shouldPaintClippingMask) {
+            // Paint the border radius mask for the fragments.
+            paintChildClippingMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
+        }
+    } // TransparencyLayerHelper block
 }
 
 static bool inContainingBlockChain(RenderLayer* startLayer, RenderLayer* endLayer)