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

Source/core/paint/LayerPainter.cpp

 // Copyright 2014 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 "config.h"
 #include "core/paint/LayerPainter.h"
 
 #include "core/frame/Settings.h"
 #include "core/page/Page.h"
 #include "core/paint/FilterPainter.h"
@@ skipping 57 matching lines @@
     if (m_renderLayer.paintsWithTransparency(paintingInfo.paintBehavior))
         paintFlags |= PaintLayerHaveTransparency;
 
     // 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 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;
         }
 
         // 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);
@@ skipping 10 matching lines @@
         }
 
         paintLayerByApplyingTransform(context, paintingInfo, paintFlags);
 
         return;
     }
 
     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())
+    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.

+}
+
+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;
 
-    // FIXME: refactor to remove this.
-    if (m_renderLayer.usedTransparency())
-        return;
-
-    m_renderLayer.setUsedTransparency(true);
     context->save();
     LayoutRect clipRect = m_renderLayer.paintingExtent(rootLayer, paintDirtyRect, subPixelAccumulation, paintBehavior);
     context->clip(clipRect);
 
     if (m_renderLayer.renderer()->hasBlendMode())
         context->setCompositeOperation(context->compositeOperation(), m_renderLayer.renderer()->style()->blendMode());
 
     context->beginTransparencyLayer(m_renderLayer.renderer()->opacity());
 
     if (m_renderLayer.renderer()->hasBlendMode())
@@ skipping 118 matching lines @@
     m_renderLayer.convertToLayerCoords(paintingInfo.rootLayer, offsetFromRoot);
 
     if (m_renderLayer.compositingState() == PaintsIntoOwnBacking)
         offsetFromRoot.move(m_renderLayer.subpixelAccumulation());
 
     LayoutRect rootRelativeBounds;
     bool rootRelativeBoundsComputed = false;
 
     ClipPathHelper clipPathHelper(context, m_renderLayer, paintingInfo, rootRelativeBounds, rootRelativeBoundsComputed, offsetFromRoot, paintFlags);
 
-    beginTransparencyLayers(context, paintingInfo.rootLayer, paintingInfo.paintDirtyRect, paintingInfo.subPixelAccumulation, paintingInfo.paintBehavior);
+    {

[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);
+        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);
+        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);
+        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;
 
-        if (shouldPaintOwnContents) {
-            paintForegroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
-                localPaintingInfo, paintBehavior, paintingRootForRenderer, selectionOnly, paintFlags);
+        { // 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 (shouldPaintNegZOrderList)
+                paintChildren(NegativeZOrderChildren, context, paintingInfo, paintFlags);
+
+            if (shouldPaintOwnContents) {
+                paintForegroundForFragments(layerFragments, context, paintingInfo.paintDirtyRect,
+                    localPaintingInfo, paintBehavior, paintingRootForRenderer, selectionOnly, paintFlags);
+            }
+
+            if (shouldPaintOutline)
+                paintOutlineForFragments(layerFragments, context, localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
+
+            if (shouldPaintNormalFlowAndPosZOrderLists)
+                paintChildren(NormalFlowChildren | PositiveZOrderChildren, context, paintingInfo, paintFlags);
+
+            if (shouldPaintOverlayScrollbars)
+                paintOverflowControlsForFragments(layerFragments, context, localPaintingInfo, paintFlags);
+        } // FilterPainter block
+
+        bool shouldPaintMask = (paintFlags & PaintLayerPaintingCompositingMaskPhase) && shouldPaintContent && m_renderLayer.renderer()->hasMask() && !selectionOnly;
+        bool shouldPaintClippingMask = (paintFlags & PaintLayerPaintingChildClippingMaskPhase) && shouldPaintContent && !selectionOnly;
+
+        if (shouldPaintMask)
+            paintMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
+        if (shouldPaintClippingMask) {
+            // Paint the border radius mask for the fragments.
+            paintChildClippingMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
         }
-
-        if (shouldPaintOutline)
-            paintOutlineForFragments(layerFragments, context, localPaintingInfo, paintBehavior, paintingRootForRenderer, paintFlags);
-
-        if (shouldPaintNormalFlowAndPosZOrderLists)
-            paintChildren(NormalFlowChildren | PositiveZOrderChildren, context, paintingInfo, paintFlags);
-
-        if (shouldPaintOverlayScrollbars)
-            paintOverflowControlsForFragments(layerFragments, context, localPaintingInfo, paintFlags);
-    } // Filter painter block
-
-    bool shouldPaintMask = (paintFlags & PaintLayerPaintingCompositingMaskPhase) && shouldPaintContent && m_renderLayer.renderer()->hasMask() && !selectionOnly;
-    bool shouldPaintClippingMask = (paintFlags & PaintLayerPaintingChildClippingMaskPhase) && shouldPaintContent && !selectionOnly;
-
-    if (shouldPaintMask)
-        paintMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
-
-    if (shouldPaintClippingMask) {
-        // Paint the border radius mask for the fragments.
-        paintChildClippingMaskForFragments(layerFragments, context, localPaintingInfo, paintingRootForRenderer, paintFlags);
-    }
-
-    // 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);
-    }
+    } // TransparencyLayerHelper block
 }
 
 static bool inContainingBlockChain(RenderLayer* startLayer, RenderLayer* endLayer)
 {
     if (startLayer == endLayer)
         return true;
 
     RenderView* view = startLayer->renderer()->view();
     for (RenderBlock* currentBlock = startLayer->renderer()->containingBlock(); currentBlock && currentBlock != view; currentBlock = currentBlock->containingBlock()) {
         if (currentBlock->layer() == endLayer)
@@ skipping 443 matching lines @@
 
         OwnPtr<ClipRecorder> clipRecorder;
         if (needsToClip(paintingInfo, clipRect))
             clipRecorder = adoptPtr(new ClipRecorder(m_renderLayer.parent(), context, DisplayItem::ClipLayerFragmentParent, clipRect));
 
         paintLayerByApplyingTransform(context, paintingInfo, paintFlags, fragment.paginationOffset);
     }
 }
 
 } // namespace blink