[webkit-reviews] review granted: [Bug 50192] Compositing overlap testing can throw layers into compositing when they should not be. : [Attachment 129927] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Adrienne Walker
<enne at google.com>'s request for review:
Bug 50192: Compositing overlap testing can throw layers into compositing when
they should not be.
https://bugs.webkit.org/show_bug.cgi?id=50192

Attachment 129927: Patch
https://bugs.webkit.org/attachment.cgi?id=129927&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129927&action=review


This looks good. I would like to see an additional test that tests > 1 level of
nesting; I'll attach something to the bug.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:123
> +    void startCompositingAncestor()
> +    {
> +	   m_overlapStack.append(Region());
> +    }
> +
> +    void endCompositingAncestor()
> +    {
> +	   m_overlapStack[m_overlapStack.size() -
2].unite(m_overlapStack.last());
> +	   m_overlapStack.removeLast();
> +    }

I think these methods could have better names. "startCompositingAncestor"
implies "start to composite the ancestor". It's really a push()/pop().

Maybe pushCompositingContainer/popCompositingContainer?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:803
> +    if (overlapMap && childState.m_compositingAncestor &&
!childState.m_compositingAncestor->isRootLayer())
> +	   addToOverlapMap(*overlapMap, layer, absBounds, haveComputedBounds);

I'd like to see a comment above these lines saying why you add this layer to
the overlap map even if it is not being composited.

> LayoutTests/compositing/layer-creation/stacking-context-overlap.html:10
> +	       -webkit-transform:translateZ(0);

Space after : please.