[META][CSS Blending] Add support for blendmode to platform code for mac

• Component: CSS
• Status: NEW ---

[Rik Cabanier]

Previous patch added support for parsing and storing the value in the engine.
This patch will add support to the platform specific code for mac so blend modes should start working for that target.

[Rik Cabanier]

Created attachment 161789 [details]
First try

[WebKit Review Bot]

Comment on attachment 161789 [details]
First try

Attachment 161789 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13732113

New failing tests:
css3/compositing/effect-blend-mode.html

[WebKit Review Bot]

Comment on attachment 161789 [details]
First try

Attachment 161789 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13729202

New failing tests:
css3/compositing/effect-blend-mode.html

[Rik Cabanier]

Created attachment 161837 [details]
updated expectations file for chromium

[Simon Fraser (smfr)]

Comment on attachment 161837 [details]
updated expectations file for chromium

View in context: https://bugs.webkit.org/attachment.cgi?id=161837&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1081
> +#if ENABLE(CSS_COMPOSITING)
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateLayersCompositing();
> +#endif

I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2782
> +void GraphicsLayerCA::updateLayersCompositing()

This should become updateBlending() or updateBlendMode(). The 'layer' in the name is redundant.

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

blending.

> Source/WebCore/rendering/RenderLayerBacking.cpp:253
> +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)

'layer' here is redundant.

[Rik Cabanier]

(In reply to comment #5)
> (From update of attachment 161837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161837&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1081
> > +#if ENABLE(CSS_COMPOSITING)
> > +    if (m_uncommittedChanges & CompositingChanged)
> > +        updateLayersCompositing();
> > +#endif
> 
> I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2782
> > +void GraphicsLayerCA::updateLayersCompositing()
> 
> This should become updateBlending() or updateBlendMode(). The 'layer' in the name is redundant.
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:383
> > +        CompositingChanged = 1 << 26,
> 
> blending.

the blendmode is only the first item.
The spec defines keywords for compositing as well and they will follow the same codepath.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> 
> 'layer' here is redundant.

I will fix this.

[Rik Cabanier]

> > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> 
> 'layer' here is redundant.

It's the same as updateLayerFilters, updateLayerOpacity & updateLayerTransform.
Should those functions change as well?


> I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?

To clarify, this function will also be used to set up the compositing modes and group isolation.

[Simon Fraser (smfr)]

(In reply to comment #7)
> > > Source/WebCore/rendering/RenderLayerBacking.cpp:253
> > > +void RenderLayerBacking::updateLayerBlendMode(const RenderStyle* style)
> > 
> > 'layer' here is redundant.
> 
> It's the same as updateLayerFilters, updateLayerOpacity & updateLayerTransform.
> Should those functions change as well?

Yes, but you don't have to do that in this patch.

> > I think it's very confusing to use the term "compositing" in code which is all about accelerated compositing. Maybe use "blending" instead?
> 
> To clarify, this function will also be used to set up the compositing modes and group isolation.

I still think using the term "compositing" unqualified in the context of GraphicsLayer is very confusing. I think a less confusing but not quite accurate name would be better.

[Rik Cabanier]

Created attachment 163315 [details]
renamed function per Simon Fraser's request

[Simon Fraser (smfr)]

Comment on attachment 163315 [details]
renamed function per Simon Fraser's request

View in context: https://bugs.webkit.org/attachment.cgi?id=163315&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2794
> +void GraphicsLayerCA::updateBlending()
> +{
> +    primaryLayer()->setBlendMode(m_blendMode);
> +    
> +    if (LayerMap* layerCloneMap = primaryLayerClones()) {
> +        LayerMap::const_iterator end = layerCloneMap->end();
> +        for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it) {
> +            if (m_replicaLayer && isReplicatedRootClone(it->first))
> +                continue;
> +            it->second->setBlendMode(m_blendMode);
> +        } 
> +    }
> +}

Does this play nicely with -webkit-transform: preserve-3d? Before, we've had spew from Core Animation when setting filters on a CATransformLayer.

[Rik Cabanier]

(In reply to comment #10)
> (From update of attachment 163315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163315&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2794
> > +void GraphicsLayerCA::updateBlending()
> > +{
> > +    primaryLayer()->setBlendMode(m_blendMode);
> > +    
> > +    if (LayerMap* layerCloneMap = primaryLayerClones()) {
> > +        LayerMap::const_iterator end = layerCloneMap->end();
> > +        for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it) {
> > +            if (m_replicaLayer && isReplicatedRootClone(it->first))
> > +                continue;
> > +            it->second->setBlendMode(m_blendMode);
> > +        } 
> > +    }
> > +}
> 
> Does this play nicely with -webkit-transform: preserve-3d? Before, we've had spew from Core Animation when setting filters on a CATransformLayer.

It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d. I did not see any warnings.

If you set a blend (or opacity or a filter) on an element, preserve-3d is supposed to be ignored but that doesn't happen today. That would be a new bug if you want me to look into that...

[Simon Fraser (smfr)]

> It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d.
preserve-3d doesn't create a 3D transform. It causes us to create a CATransformLayer, which may not be able to handle compositing filters.

[Rik Cabanier]

(In reply to comment #12)
> > It plays along fairly well. If you set the blend mode on an object that has a 3d transform, it will blend in 3d.
> preserve-3d doesn't create a 3D transform. It causes us to create a CATransformLayer, which may not be able to handle compositing filters.

I morphed the example from http://www.webkit.org/blog-files/3d-transforms/transform-style.html.
What I see is that blending is limited to within the group that has the preserve-3d applied. If I apply it on the parent, it doesn't work.

I don't think that that's a big deal because we should fix the bug that blending/opacity/filters removes the effect of the preserve-3d flag.

[Simon Fraser (smfr)]

Comment on attachment 163315 [details]
renamed function per Simon Fraser's request

View in context: https://bugs.webkit.org/attachment.cgi?id=163315&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1080
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateBlending();

The 'changed' enum name should match the updateFoo.

> LayoutTests/css3/compositing/effect-blend-mode-expected.txt:3
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600

Is this useful as render tree ouput?

[Simon Fraser (smfr)]

Also, this isn't going to work in WebKit1, where parts of the page content are rendered into the window, outside of Core Animation.

[Rik Cabanier]

Created attachment 167643 [details]
Updated patch because it became out-of-date

[Rik Cabanier]

Created attachment 167644 [details]
Updated patch because previous version was out-of-date

[WebKit Review Bot]

Comment on attachment 167644 [details]
Updated patch because previous version was out-of-date

Attachment 167644 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14204899

New failing tests:
css3/compositing/effect-blend-mode.html

[Build Bot]

Comment on attachment 167644 [details]
Updated patch because previous version was out-of-date

Attachment 167644 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14211600

New failing tests:
css3/compositing/effect-blend-mode.html

[Radar WebKit Bug Importer]

<rdar://problem/96961828>