[webkit-reviews] review denied: [Bug 99200] Add support for blendmode to Core Animation layer : [Attachment 173191] Updated patch because it became out-of-date

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Rik Cabanier
<cabanier at adobe.com>'s request for review:
Bug 99200: Add support for blendmode to Core Animation layer
https://bugs.webkit.org/show_bug.cgi?id=99200

Attachment 173191: Updated patch because it became out-of-date
https://bugs.webkit.org/attachment.cgi?id=173191&action=review

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


This is pretty close. I'm not convinced the m_subtreeHasBlending = true;  works
without seeing some more tests, and wonder if it will cause every subsequent
layer in the tree traversal to be composited.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2935
> +    primaryLayer()->setBlendMode(m_blendMode);

I recently fixed an issue with reflections where primary() layer was the wrong
layer to use. Please check reflected, blended content.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:395
> +	   BlendingModeChanged = 1 << 26,

You're re-using 26.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:900
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +	   compositingState.m_subtreeHasBlending = true;
> +
>      // Now check for reasons to become composited that depend on the state
of descendant layers.
>      RenderLayer::IndirectCompositingReason indirectCompositingReason;
>      if (!willBeComposited && canBeComposited(layer)
> -	   && requiresCompositingForIndirectReason(layer->renderer(),
childState.m_subtreeIsCompositing, anyDescendantHas3DTransform,
indirectCompositingReason)) {
> +	   && requiresCompositingForIndirectReason(layer->renderer(),
childState.m_subtreeIsCompositing, compositingState.m_subtreeHasBlending,
anyDescendantHas3DTransform, indirectCompositingReason)) {
>	   layer->setIndirectCompositingReason(indirectCompositingReason);
>	   childState.m_compositingAncestor = layer;

You should add some tests that exercise this code.

> Source/WebCore/rendering/RenderLayerCompositor.h:318
> +    bool requiresCompositingForIndirectReason(RenderObject*, bool , 
> +	   bool , bool , RenderLayer::IndirectCompositingReason&) const;

Those bool parameter names are useful; I think you should keep them.