Don't coerce pointers to compositor layer mappings to booleans.

Source/core/rendering/RenderLayer.cpp

Index: Source/core/rendering/RenderLayer.cpp
diff --git a/Source/core/rendering/RenderLayer.cpp b/Source/core/rendering/RenderLayer.cpp
index 8bfd94b6ff48e5e07fbd519fb5131f2e4c81dcb8..78d440c9fc8dc258c2412c2ff82264d0ad74732f 100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -288,8 +288,10 @@ void RenderLayer::updateLayerPositions(RenderGeometryMap* geometryMap, UpdateLay
         m_reflectionInfo->reflection()->layout();
 
     // Clear the IsCompositingUpdateRoot flag once we've found the first compositing layer in this update.
+    // Layers are positioned in the space of the composited layer they paint into, so we only clear
+    // IsCompositingUpdateRoot when we reach a layer that paints into its own backing.
     bool isUpdateRoot = (flags & IsCompositingUpdateRoot);
-    if (compositedLayerMapping())
+    if (compositingState() == PaintsIntoOwnBacking)
         flags &= ~IsCompositingUpdateRoot;
 
     if (useRegionBasedColumns() && renderer()->isInFlowRenderFlowThread()) {
@@ -303,7 +305,7 @@ void RenderLayer::updateLayerPositions(RenderGeometryMap* geometryMap, UpdateLay
     for (RenderLayer* child = firstChild(); child; child = child->nextSibling())
         child->updateLayerPositions(geometryMap, flags);
 
-    if ((flags & UpdateCompositingLayers) && compositedLayerMapping()) {
+    if ((flags & UpdateCompositingLayers) && compositingState() == PaintsIntoOwnBacking) {
         CompositedLayerMapping::UpdateAfterLayoutFlags updateFlags = CompositedLayerMapping::CompositingChildrenOnly;
         if (flags & NeedsFullRepaintInBacking)
             updateFlags |= CompositedLayerMapping::NeedsFullRepaint;
@@ -535,7 +537,8 @@ void RenderLayer::updateBlendMode()
         if (!hadBlendMode || !hasBlendMode())
             dirtyAncestorChainBlendedDescendantStatus();
 
-        if (compositedLayerMapping())
+        // The compositor can only apply one blend mode per composited layer.
+        if (compositingState() == PaintsIntoOwnBacking)
             compositedLayerMapping()->setBlendMode(newBlendMode);
     }
 }
@@ -642,7 +645,7 @@ void RenderLayer::updatePagination()
     m_isPaginated = false;
     m_enclosingPaginationLayer = 0;
 
-    if (compositedLayerMapping() || !parent())
+    if (compositingState() == PaintsIntoOwnBacking || !parent())

[shawnsingh, 2013/11/05 10:20:27]

I'm really not sure about this one.  If you feel certain, can you please
elaborate why we shouldn't include HasOwnBackingButPaintsIntoAncestor here?

         return; // FIXME: We will have to deal with paginated compositing layers someday.
                 // FIXME: For now the RenderView can't be paginated.  Eventually printing will move to a model where it is though.
 
@@ -993,7 +996,10 @@ bool RenderLayer::updateLayerPosition()
             localPoint += offset;
         }
     } else if (parent()) {
-        if (compositedLayerMapping()) {
+        // Rationale: composited layers need to be positioned in their correct column. If we've been squashed into another layer,
+        // it will already be positioned into the column-correct location. If we also adjust our position, we'll be double
+        // compensating.
+        if (compositingState() == PaintsIntoOwnBacking) {

[shawnsingh, 2013/11/05 10:20:27]

What about HasOwnBackingButPaintsIntoAncestor ?   composited layer positioning
doesn't care about whether contents paint into it or an ancestor, right?

[Ian Vollick, 2013/11/08 03:35:12]

That was the point I was trying to address with this comment: if we paint into a
composited layer that is already in its column-correct location, executing this
code will be a double compensation. The code below is quite hairy and I may be
misunderstanding it; but that's my current impression.

             // FIXME: Composited layers ignore pagination, so about the best we can do is make sure they're offset into the appropriate column.
             // They won't split across columns properly.
             LayoutSize columnOffset;
@@ -1113,11 +1119,11 @@ static inline const RenderLayer* compositingContainer(const RenderLayer* layer)
 // enclosingCompositingLayerForRepaint().
 RenderLayer* RenderLayer::enclosingCompositingLayer(bool includeSelf) const
 {
-    if (includeSelf && compositedLayerMapping())
+    if (includeSelf && compositingState() != NotComposited)
         return const_cast<RenderLayer*>(this);
 
     for (const RenderLayer* curr = compositingContainer(this); curr; curr = compositingContainer(curr)) {
-        if (curr->compositedLayerMapping())
+        if (curr->compositingState() != NotComposited)
             return const_cast<RenderLayer*>(curr);
     }
 
@@ -1191,7 +1197,7 @@ bool RenderLayer::hasAncestorWithFilterOutsets() const
 
 RenderLayer* RenderLayer::clippingRootForPainting() const
 {
-    if (compositedLayerMapping())
+    if (compositingState() == PaintsIntoOwnBacking)

[shawnsingh, 2013/11/05 10:20:27]

Should this also include HasOwnBackingButPaintsIntoAncestor ?  I can imagine a
composited layer that paints into ancestor but still has a clip.  That's one of
the common cases where a composited layer paints into its ancestor in the first
place.

         return const_cast<RenderLayer*>(this);
 
     const RenderLayer* current = this;
@@ -1230,11 +1236,14 @@ bool RenderLayer::isTransparent() const
 
 RenderLayer* RenderLayer::transparentPaintingAncestor()
 {
-    if (compositedLayerMapping())
+    // The compositor will handle applying opacity for true composited layers.
+    // If we squish into a layer that is not transparent, we won't be able to
+    // rely on it to do this.
+    if (compositingState() == PaintsIntoOwnBacking)

[shawnsingh, 2013/11/05 10:20:27]

Again, I think we need to include HasOwnBackingbutPaintsIntoAncestor here...
it's about the composited layer hierarchy, not about who paints into where...

The scenario here is a composited layer with a non-composited ancestor that has
an opacity.  The composited layer has to walk all the way up to the next
composited layer to accumulate the correct opacity that it must inherit from all
non-composited layers.  The walk stops at the first layer that shows up in the
composited layer tree - regardless of whether it paints into its own backing or
not.

         return 0;
 
     for (RenderLayer* curr = parent(); curr; curr = curr->parent()) {
-        if (curr->compositedLayerMapping())
+        if (curr->compositingState() == PaintsIntoOwnBacking)
             return 0;
         if (curr->isTransparent())
             return curr;
@@ -1770,8 +1779,8 @@ void RenderLayer::updateScrollableArea()
 
 PassOwnPtr<Vector<FloatRect> > RenderLayer::collectTrackedRepaintRects() const
 {
-    if (CompositedLayerMapping* mapping = compositedLayerMapping())
-        return mapping->collectTrackedRepaintRects();
+    if (compositingState() == PaintsIntoOwnBacking)

[shawnsingh, 2013/11/05 10:20:27]

So yeah, the theme of my review - did you remember to use
HasOwnBackingButPaintsIntoAncestor? =P  Or am I missing something that makes you
confident we don't need it in so many places? 

+        return compositedLayerMapping()->collectTrackedRepaintRects();
     return nullptr;
 }
 
@@ -3589,7 +3598,9 @@ IntRect RenderLayer::calculateLayerBounds(const RenderLayer* ancestorLayer, cons
 
     if (m_reflectionInfo) {
         RenderLayer* reflectionLayer = m_reflectionInfo->reflectionLayer();
-        if (!reflectionLayer->compositedLayerMapping()) {
+        // If the reflection layer paints either into its compositing ancestor
+        // or is not composited all, we have to consider its effect on our bounds.
+        if (reflectionLayer->compositingState() != PaintsIntoOwnBacking) {
             IntRect childUnionBounds = reflectionLayer->calculateLayerBounds(this, 0, descendantFlags);
             unionBounds.unite(childUnionBounds);
         }
@@ -3601,12 +3612,9 @@ IntRect RenderLayer::calculateLayerBounds(const RenderLayer* ancestorLayer, cons
     LayerListMutationDetector mutationChecker(const_cast<RenderLayer*>(this)->stackingNode());
 #endif
 
-    // FIXME: Descendants that are composited should not necessarily be skipped, if they don't paint into their own
-    // separate backing. Instead, they ought to contribute to the bounds of the layer we're trying to compute.
-    // This applies to all z-order lists below.
     RenderLayerStackingNodeIterator iterator(*m_stackingNode.get(), AllChildren);
     while (RenderLayerStackingNode* node = iterator.next()) {
-        if (flags & IncludeCompositedDescendants || !node->layer()->compositedLayerMapping()) {
+        if (flags & IncludeCompositedDescendants || node->layer()->compositingState() != PaintsIntoOwnBacking) {

[shawnsingh, 2013/11/05 10:20:27]

This was another place where I put a FIXME because it's very likely a bug that
we should repro with a layout test before fixing.  Could we add a layout test
that repros a bug before your patch?

[Ian Vollick, 2013/11/08 03:35:12]

Sure. I've reinstated the FIXME and matched the existing, buggy behavior.

             IntRect childUnionBounds = node->layer()->calculateLayerBounds(this, 0, descendantFlags);
             unionBounds.unite(childUnionBounds);
         }
@@ -3649,7 +3657,7 @@ CompositingState RenderLayer::compositingState() const
     return PaintsIntoOwnBacking;
 }
 
-CompositedLayerMapping* RenderLayer::ensureCompositedLayerMapping()
+CompositedLayerMappingPtr RenderLayer::ensureCompositedLayerMapping()
 {
     if (!m_compositedLayerMapping) {
         m_compositedLayerMapping = adoptPtr(new CompositedLayerMapping(this));
@@ -3736,7 +3744,7 @@ bool RenderLayer::listBackgroundIsKnownToBeOpaqueInRect(const Vector<RenderLayer
 
     for (Vector<RenderLayerStackingNode*>::const_reverse_iterator iter = list->rbegin(); iter != list->rend(); ++iter) {
         const RenderLayer* childLayer = (*iter)->layer();
-        if (childLayer->compositedLayerMapping())
+        if (childLayer->compositingState() == PaintsIntoOwnBacking)
             continue;
 
         if (!childLayer->canUseConvertToLayerCoords())
@@ -3909,7 +3917,7 @@ inline bool RenderLayer::needsCompositingLayersRebuiltForClip(const RenderStyle*
 inline bool RenderLayer::needsCompositingLayersRebuiltForOverflow(const RenderStyle* oldStyle, const RenderStyle* newStyle) const
 {
     ASSERT(newStyle);
-    return !compositedLayerMapping() && oldStyle && (oldStyle->overflowX() != newStyle->overflowX()) && m_stackingNode->ancestorStackingContainerNode()->layer()->hasCompositingDescendant();
+    return compositingState() == NotComposited && oldStyle && (oldStyle->overflowX() != newStyle->overflowX()) && m_stackingNode->ancestorStackingContainerNode()->layer()->hasCompositingDescendant();
 }
 
 inline bool RenderLayer::needsCompositingLayersRebuiltForFilters(const RenderStyle* oldStyle, const RenderStyle* newStyle, bool didPaintWithFilters) const
@@ -3953,7 +3961,7 @@ void RenderLayer::updateFilters(const RenderStyle* oldStyle, const RenderStyle*
     updateOrRemoveFilterClients();
     // During an accelerated animation, both WebKit and the compositor animate properties.
     // However, WebKit shouldn't ask the compositor to update its filters if the compositor is performing the animation.
-    bool shouldUpdateFilters = compositedLayerMapping() && !renderer()->animation().isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyWebkitFilter);
+    bool shouldUpdateFilters = compositingState() == PaintsIntoOwnBacking && !renderer()->animation().isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyWebkitFilter);
     if (shouldUpdateFilters)
         compositedLayerMapping()->updateFilters(renderer()->style());
     updateOrRemoveFilterEffectRenderer();
@@ -3992,7 +4000,7 @@ void RenderLayer::styleChanged(StyleDifference, const RenderStyle* oldStyle)
         || needsCompositingLayersRebuiltForOverflow(oldStyle, newStyle)
         || needsCompositingLayersRebuiltForFilters(oldStyle, newStyle, didPaintWithFilters))
         compositor()->setCompositingLayersNeedRebuild();
-    else if (compositedLayerMapping())
+    else if (compositingState() == PaintsIntoOwnBacking)

[shawnsingh, 2013/11/05 10:20:27]

I'm pretty sure we would need HasOwnBackingButPaintsIntoAncestor here, too?

         compositedLayerMapping()->updateGraphicsLayerGeometry();
 }