Bug 99200 – Implement compositing path for blending

• Component: CSS
• Status: RESOLVED FIXED

[Rik Cabanier]

As a first pass, always assume that blending creates a layer.
Apply blending to this layer.

[Rik Cabanier]

Created attachment 168530 [details]
Patch

[Rik Cabanier]

Created attachment 168532 [details]
Patch

[Build Bot]

Comment on attachment 168532 [details]
Patch

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

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

[Rik Cabanier]

Created attachment 168536 [details]
Patch

[WebKit Review Bot]

Attachment 168536 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:676:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/chromium/TestExpectations:677:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

Created attachment 168537 [details]
strange failure. Try again for EWS output

[WebKit Review Bot]

Attachment 168537 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:676:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/chromium/TestExpectations:677:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

Created attachment 168574 [details]
Patch

[Rik Cabanier]

Created attachment 168585 [details]
Try again to get linux chromium bot feedback

[Simon Fraser (smfr)]

Comment on attachment 168574 [details]
Patch

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

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

Confusing that you called the flag CompositingChanged but then call updateBlending().

> Source/WebCore/rendering/RenderLayerBacking.cpp:1389
> +    if (m_graphicsLayer)
> +        m_graphicsLayer->setBlendMode(blendMode);

No need to null-check m_graphicsLayer.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +        compositingState.m_subtreeHasBlending = true;

I don't think you need to track this at all.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1805
> +    bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason& reason) const

HasBlendedDescendants -> hasBlendedDescendants

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1812
> -    if (hasCompositedDescendants && (layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> +    if (hasCompositedDescendants
> +        && (HasBlendedDescendants || layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {

You have this backwards. A layer with blending needs to be composited if has composited descendants, but not vice versa. It should be just like opacity and filters, which are consulted in createsGroup().

> LayoutTests/css3/compositing/effect-blend-mode-expected.txt:38
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +      RenderBlock {UL} at (0,0) size 784x0
> +        RenderBlock (floating) {LI} at (45,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,425) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130

This output is not useful. It should be a dumpAsText(true) test.

> LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.txt:53
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x576
> +  RenderBlock {HTML} at (0,0) size 800x576
> +    RenderBody {BODY} at (8,16) size 784x0
> +      RenderBlock {UL} at (0,0) size 784x0
> +        RenderBlock (floating) {LI} at (45,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,5) size 130x130
> +        RenderBlock (floating) {LI} at (325,5) size 130x130
> +        RenderBlock (floating) {LI} at (465,5) size 130x130
> +        RenderBlock (floating) {LI} at (605,5) size 130x130
> +        RenderBlock (floating) {LI} at (45,145) size 130x130
> +        RenderBlock (floating) {LI} at (185,145) size 130x130
> +        RenderBlock (floating) {LI} at (325,145) size 130x130
> +        RenderBlock (floating) {LI} at (465,145) size 130x130
> +        RenderBlock (floating) {LI} at (605,145) size 130x130
> +        RenderBlock (floating) {LI} at (45,285) size 130x130
> +        RenderBlock (floating) {LI} at (185,285) size 130x130
> +        RenderBlock (floating) {LI} at (325,285) size 130x130
> +        RenderBlock (floating) {LI} at (465,285) size 130x130
> +        RenderBlock (floating) {LI} at (605,285) size 130x130
> +        RenderBlock (floating) {LI} at (45,425) size 130x130
> +layer at (193,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (193,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (193,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,441) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130

Ditto.

> LayoutTests/platform/mac/css3/compositing/should-have-compositing-layer-expected.txt:1
> +Test to make sure a blend mode creates a compositing layer. Test is successful of render tree shows compositing

You need a test that has blending on an element with composited children, and dumps the layer tree too.

[Rik Cabanier]

(In reply to comment #10)
> (From update of attachment 168574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168574&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1126
> > +    if (m_uncommittedChanges & CompositingChanged)
> > +        updateBlending();
> 
> Confusing that you called the flag CompositingChanged but then call updateBlending().

the idea is that this will be extended later for compositing as well.
I can rename the flag for now if you want to

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:1389
> > +    if (m_graphicsLayer)
> > +        m_graphicsLayer->setBlendMode(blendMode);
> 
> No need to null-check m_graphicsLayer.

OK

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> > +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> > +        compositingState.m_subtreeHasBlending = true;
> 
> I don't think you need to track this at all.

Unfortunately, it does seem like it's needed.
If there's no backing for the root layer, the top layer will not blend with it.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1805
> > +    bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason& reason) const
> 
> HasBlendedDescendants -> hasBlendedDescendants
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1812
> > -    if (hasCompositedDescendants && (layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> > +    if (hasCompositedDescendants
> > +        && (HasBlendedDescendants || layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> 
> You have this backwards. A layer with blending needs to be composited if has composited descendants, but not vice versa. It should be just like opacity and filters, which are consulted in createsGroup().

See above

> 
> > LayoutTests/css3/compositing/effect-blend-mode-expected.txt:38
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x600
> > +  RenderBlock {HTML} at (0,0) size 800x600
> > +    RenderBody {BODY} at (8,8) size 784x584
> > +      RenderBlock {UL} at (0,0) size 784x0
> > +        RenderBlock (floating) {LI} at (45,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,425) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> 
> This output is not useful. It should be a dumpAsText(true) test.

I copied the test from the filters folder.
Will update.

> 
> > LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.txt:53
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x576
> > +  RenderBlock {HTML} at (0,0) size 800x576
> > +    RenderBody {BODY} at (8,16) size 784x0
> > +      RenderBlock {UL} at (0,0) size 784x0
> > +        RenderBlock (floating) {LI} at (45,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,5) size 130x130
> > +        RenderBlock (floating) {LI} at (325,5) size 130x130
> > +        RenderBlock (floating) {LI} at (465,5) size 130x130
> > +        RenderBlock (floating) {LI} at (605,5) size 130x130
> > +        RenderBlock (floating) {LI} at (45,145) size 130x130
> > +        RenderBlock (floating) {LI} at (185,145) size 130x130
> > +        RenderBlock (floating) {LI} at (325,145) size 130x130
> > +        RenderBlock (floating) {LI} at (465,145) size 130x130
> > +        RenderBlock (floating) {LI} at (605,145) size 130x130
> > +        RenderBlock (floating) {LI} at (45,285) size 130x130
> > +        RenderBlock (floating) {LI} at (185,285) size 130x130
> > +        RenderBlock (floating) {LI} at (325,285) size 130x130
> > +        RenderBlock (floating) {LI} at (465,285) size 130x130
> > +        RenderBlock (floating) {LI} at (605,285) size 130x130
> > +        RenderBlock (floating) {LI} at (45,425) size 130x130
> > +layer at (193,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (193,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (193,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,441) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> 
> Ditto.
> 
> > LayoutTests/platform/mac/css3/compositing/should-have-compositing-layer-expected.txt:1
> > +Test to make sure a blend mode creates a compositing layer. Test is successful of render tree shows compositing
> 
> You need a test that has blending on an element with composited children, and dumps the layer tree too.

This will be updated when integrating CG. For now everything with blending creates a compositing layer (as we agreed last week)

[Rik Cabanier]

Created attachment 168746 [details]
Fixes per smfr

[Eric Seidel (no email)]

Attachment 168746 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

Created attachment 168793 [details]
try again. bad style bot

[Eric Seidel (no email)]

Attachment 168793 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

Created attachment 168847 [details]
I set up my own bot

[Rik Cabanier]

Created attachment 168849 [details]
cleaned my tree for my bot

[Rik Cabanier]

Attachment 168849 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/TestExpectat..." exit_code: 1
WARNING: Patch's size is 23 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/resources/ducky.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/resources/ducky.png).  [image/png] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

Created attachment 168853 [details]
set the mime type

[Rik Cabanier]

Attachment 168853 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/TestExpectat..." exit_code: 1
WARNING: Patch's size is 23 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/resources/ducky.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/resources/ducky.png).  [image/png] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

So much noise.....

[Rik Cabanier]

Created attachment 168965 [details]
Finally figured it out. Sorry for the noise!

[Alexandru Chiculita]

Comment on attachment 168965 [details]
Finally figured it out. Sorry for the noise!

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

Looks good overall! Some minor comments below.

> Source/WebCore/platform/graphics/GraphicsLayer.h:316
> +

nit: Remove the empty line here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
> +    noteLayerPropertyChanged(CompositingChanged);

I think CompositingChanged should actually be BlendingModeChanged.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:678
> +    CIFilter* filter = nil;

Do you need a RetainPtr here? Looking at the filters function below it looks like only the array has a RetainPtr and the filters don't, but that might be a bug too.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +        compositingState.m_subtreeHasBlending = true;

Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.

> Source/WebCore/rendering/RenderLayerCompositor.h:303
> +        bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason&) const;

nit: Use lower case first letter in parameter names.

> LayoutTests/css3/compositing/effect-blend-mode.html:1
> +<!DOCTYPE HTML>

I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?

> LayoutTests/css3/compositing/effect-blend-mode.html:24
> +<ul>

You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.

Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.

> LayoutTests/platform/chromium/TestExpectations:112
> +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]

What about other platforms? Are they skipping the whole compositing folder already?

[Rik Cabanier]

(In reply to comment #23)
> (From update of attachment 168965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168965&action=review
> 
> Looks good overall! Some minor comments below.
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:316
> > +
> 
> nit: Remove the empty line here.

Will do

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
> > +    noteLayerPropertyChanged(CompositingChanged);
> 
> I think CompositingChanged should actually be BlendingModeChanged.

This is in preparation for compositing since it will use the same codepath.
I can change if needed though, but that should be another patch.

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:678
> > +    CIFilter* filter = nil;
> 
> Do you need a RetainPtr here? Looking at the filters function below it looks like only the array has a RetainPtr and the filters don't, but that might be a bug too.

I don't know. As you say, this code is in spirit of how filters are implemented. I will add a retainptr and see what happens.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> > +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> > +        compositingState.m_subtreeHasBlending = true;
> 
> Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.

I don't think so. You want to blend with everything that came before so it all needs to be in a layer with backing.
The spec is unclear if this blending should happen with the page's background color, but for now it does.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:303
> > +        bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason&) const;
> 
> nit: Use lower case first letter in parameter names.

Will do.

> 
> > LayoutTests/css3/compositing/effect-blend-mode.html:1
> > +<!DOCTYPE HTML>
> 
> I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
> 1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
> 2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
> 3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
> 4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?

The next step is to add support for CG blending which will introduce a bunch of new test cases.
I'm unsure if adding a bunch of tests that will need to change again new is a good use of time.

> 
> > LayoutTests/css3/compositing/effect-blend-mode.html:24
> > +<ul>
> 
> You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.

Will do

> 
> Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.

No. This is how the file appears on platforms that don't have blending (such as the QT ports)

> 
> > LayoutTests/platform/chromium/TestExpectations:112
> > +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]
> 
> What about other platforms? Are they skipping the whole compositing folder already?

Chromium skips the folder but all the others don't

[Rik Cabanier]

Created attachment 169204 [details]
incorporated Alex' comments

[Alexandru Chiculita]

Comment on attachment 168965 [details]
Finally figured it out. Sorry for the noise!

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

>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
>>> +    noteLayerPropertyChanged(CompositingChanged);
>> 
>> I think CompositingChanged should actually be BlendingModeChanged.
> 
> This is in preparation for compositing since it will use the same codepath.
> I can change if needed though, but that should be another patch.

Then maybe you need to call it that way. Anyway, why would it require a different patch? You're adding CompositingChanged in this patch :)

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

Here's why I don't like the naming of 'CompositingChanged'. There's a compositing change, but you update the blending instead.

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
>>> +        compositingState.m_subtreeHasBlending = true;
>> 
>> Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.
> 
> I don't think so. You want to blend with everything that came before so it all needs to be in a layer with backing.
> The spec is unclear if this blending should happen with the page's background color, but for now it does.

I got that, but I still think you don't need to create compositing layers out of every single parent. If you think about the ancestors that need compositing, only a single one needs to create a layer too. That's either the root, or if you want to optimize it you could find the parent that covers the bounding box of the child and is known to be opaque.

>>> LayoutTests/css3/compositing/effect-blend-mode.html:1
>>> +<!DOCTYPE HTML>
>> 
>> I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
>> 1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
>> 2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
>> 3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
>> 4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?
> 
> The next step is to add support for CG blending which will introduce a bunch of new test cases.
> I'm unsure if adding a bunch of tests that will need to change again new is a good use of time.

What do you expect to change? 

If you add CG blending, most probably you will have a switch forcing the related tests to use CG, so the CA part will not be covered by those tests anyway. You might need to duplicate tests for the different implementation paths (software path vs. hardware path).

>>> LayoutTests/css3/compositing/effect-blend-mode.html:24
>>> +<ul>
>> 
>> You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.
>> 
>> Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.
> 
> Will do

> No. This is how the file appears on platforms that don't have blending (such as the QT ports)

In those cases you skip the test instead. Do not integrate bogus expected results. If a platform passes a test it means that the tested functionality is implemented.

>>> LayoutTests/platform/chromium/TestExpectations:112
>>> +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]
>> 
>> What about other platforms? Are they skipping the whole compositing folder already?
> 
> Chromium skips the folder but all the others don't

Are the other platforms passing the test? If they don't pass, then those platforms should skip the test/folder, too.

BTW, I think the skipped line should reference a bug that will track and fix the issue. You can add a master bug "Implement CSS3 compositing" that will track this issue globally on all the platforms.

[Rik Cabanier]

Created attachment 169274 [details]
Patch

[Rik Cabanier]

Created attachment 169287 [details]
removed retainptr

[Rik Cabanier]

Created attachment 169308 [details]
Patch

[Rik Cabanier]

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

[Simon Fraser (smfr)]

Comment on attachment 173191 [details]
Updated patch because it became out-of-date

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.

[Rik Cabanier]

Created attachment 174241 [details]
Patch

[Rik Cabanier]

Created attachment 174247 [details]
Added test files + fixed type

[Rik Cabanier]

(In reply to comment #31)
> (From update of attachment 173191 [details])
> 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.

I added anotther test that verifies that blending is also happening correctly if it is in a stacking context or if it contains a stacking context

I also looked into your other concern that all parent layers are composited. That was correct.
I added some code to RenderLayerCompositor.cpp that catches this and will turn off the tracking of blending when it's no longer needed.

> 
> > 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.

I added a testfile that blends reflected content and it seems to work OK.

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:395
> > +        BlendingModeChanged = 1 << 26,
> 
> You're re-using 26.

Oops. Yes, things changed and I didn't catch that merge

> 
> > 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.

All blending goes through 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.

I put them back in.

[Rik Cabanier]

Created attachment 174288 [details]
Made testfile more clear

[Rik Cabanier]

Created attachment 189576 [details]
refreshed patch

[Simon Fraser (smfr)]

CA has no support for non-separable modes, btw.

[Rik Cabanier]

(In reply to comment #37)
> CA has no support for non-separable modes, btw.

I believe it does (see expected png file). What is not supported is if you create a cgcontext with an iosurface.

[Rik Cabanier]

Created attachment 200723 [details]
Patch

[Mihai Tica]

Created attachment 203313 [details]
Patch

[Build Bot]

Comment on attachment 203313 [details]
Patch

Attachment 203313 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/759061

New failing tests:
css3/compositing/should-have-compositing-layer.html

[Build Bot]

Created attachment 203326 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Mihai Tica]

Created attachment 203342 [details]
Patch

[Mihai Tica]

Simon, can you please have another look at this? Thanks!

[Simon Fraser (smfr)]

Comment on attachment 203342 [details]
Patch

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

r-. We have to be able to do this without CIFilters.

> Source/WebCore/ChangeLog:3
> +        Rebasing patch.

Not a useful change log comment.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:104
> +    virtual void setBlendMode(BlendMode);

OVERRIDE

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:792
> +    [m_layer.get() setCompositingFilter:filter];

We can't use CI filters without extra work, because on Mavericks it will kick the WebProcess out of WindowServer-hosted compositing, and we need to add code to deal with the switch to fix plug-ins etc.

[Dean Jackson]

We *might* be able to do some of the effects by exporting some private CoreAnimation API, but we'd have to make sure it plays nicely with the existing filter code.

CA_EXTERN NSString * const kCAFilterNormalBlendMode;
CA_EXTERN NSString * const kCAFilterMultiplyBlendMode;
CA_EXTERN NSString * const kCAFilterScreenBlendMode;
CA_EXTERN NSString * const kCAFilterOverlayBlendMode;
CA_EXTERN NSString * const kCAFilterDarkenBlendMode;
CA_EXTERN NSString * const kCAFilterLightenBlendMode;
CA_EXTERN NSString * const kCAFilterColorDodgeBlendMode;
CA_EXTERN NSString * const kCAFilterColorBurnBlendMode;
CA_EXTERN NSString * const kCAFilterSoftLightBlendMode;
CA_EXTERN NSString * const kCAFilterHardLightBlendMode;
CA_EXTERN NSString * const kCAFilterDifferenceBlendMode;
CA_EXTERN NSString * const kCAFilterExclusionBlendMode;

CA_EXTERN NSString * const kCAFilterSubtractBlendMode;
CA_EXTERN NSString * const kCAFilterLinearBurnBlendMode;
CA_EXTERN NSString * const kCAFilterLinearDodgeBlendMode;
CA_EXTERN NSString * const kCAFilterLinearLightBlendMode;
CA_EXTERN NSString * const kCAFilterPinLightBlendMode;

[Mihai Tica]

(In reply to comment #46)
> We *might* be able to do some of the effects by exporting some private CoreAnimation API, but we'd have to make sure it plays nicely with the existing filter code.
> 
> CA_EXTERN NSString * const kCAFilterNormalBlendMode;
> CA_EXTERN NSString * const kCAFilterMultiplyBlendMode;
> CA_EXTERN NSString * const kCAFilterScreenBlendMode;
> CA_EXTERN NSString * const kCAFilterOverlayBlendMode;
> CA_EXTERN NSString * const kCAFilterDarkenBlendMode;
> CA_EXTERN NSString * const kCAFilterLightenBlendMode;
> CA_EXTERN NSString * const kCAFilterColorDodgeBlendMode;
> CA_EXTERN NSString * const kCAFilterColorBurnBlendMode;
> CA_EXTERN NSString * const kCAFilterSoftLightBlendMode;
> CA_EXTERN NSString * const kCAFilterHardLightBlendMode;
> CA_EXTERN NSString * const kCAFilterDifferenceBlendMode;
> CA_EXTERN NSString * const kCAFilterExclusionBlendMode;
> 
> CA_EXTERN NSString * const kCAFilterSubtractBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearBurnBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearDodgeBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearLightBlendMode;
> CA_EXTERN NSString * const kCAFilterPinLightBlendMode;

I've tried using these filters without success, could you please provide some pointers in using them? Are there some specific properties that have to be set in order to make them work? I'm thinking of the setValue: forKey: selector

[Dean Jackson]

Yeah, we'll have to expose the SPI for you to have access. I'll take a look.

[Dean Jackson]

Created attachment 214027 [details]
Patch

[Dean Jackson]

Here's a patch that works on Mac (Mountain Lion and Mavericks, but not Lion).

We can only support the separable blend modes in compositing. I've left the four non-separable modes unimplemented.

I'd like to do some testing on more complex content before actually deciding to land this or not. We still don't have any idea how to solve the problem of an intermediary compositing layer getting added to the tree, breaking our blending into our parent. A good example of this is <video>, which could be a bare compositing layer, or could be a hierarchy that contains sublayers for the controls.

[Dean Jackson]

Created attachment 214039 [details]
Test case showing problem

Here's a test case that shows the major issue.
Notice the difference between example 3 and 4. In this case the blending element's parent has been composited. The blending still works fine into the parent, but does not blend above that. I think you want 3 in all cases (I think trying to describe why 4 happened would be difficult, and it could happen just because an animation is running on a sibling.

[Dean Jackson]

Created attachment 214040 [details]
Screenshot of test case

[Mihai Tica]

Great work!

(In reply to comment #50)
> Here's a patch that works on Mac (Mountain Lion and Mavericks, but not Lion).
Would falling back to CIFilters when detecting Lion be feasible?
> 
> We can only support the separable blend modes in compositing. I've left the four non-separable modes unimplemented.
> 
> I'd like to do some testing on more complex content before actually deciding to land this or not. We still don't have any idea how to solve the problem of an intermediary compositing layer getting added to the tree, breaking our blending into our parent. A good example of this is <video>, which could be a bare compositing layer, or could be a hierarchy that contains sublayers for the controls.

[Mihai Tica]

(In reply to comment #51)
> Created an attachment (id=214039) [details]
> Test case showing problem
> 
> Here's a test case that shows the major issue.
> Notice the difference between example 3 and 4. In this case the blending element's parent has been composited. The blending still works fine into the parent, but does not blend above that. I think you want 3 in all cases (I think trying to describe why 4 happened would be difficult, and it could happen just because an animation is running on a sibling.

According to the spec, an element with mix-blend-mode (-webkit-blend-mode) specified should only blend with the stacking context right below it.
In the testcase attached, in example 4, the transform property creates a new stacking context for the background element, so it's actually correct for the topmost element to only blend with the background, and not the body of the document.

You can browse the spec (http://dev.w3.org/fxtf/compositing-1/#mix-blend-mode) at example 6, where this behaviour is described.

Also, we should probably make sure that this rule is followed before landing the patch and probably validate this with a few more tests. Please let me know if I can help with anything

[Simon Fraser (smfr)]

Things to test/fix:
* parent stacking context is "paints into ancestor"
* compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
* compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.

If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?

[Rik Cabanier]

(In reply to comment #55)
> Things to test/fix:
> * parent stacking context is "paints into ancestor"
> * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.

Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?

> 
> If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?

Yes. It should only blend with the stacking context ancestor.

[Dean Jackson]

I think we should land this now and add extra test cases in a follow-up (which I'm preparing now).

[Simon Fraser (smfr)]

(In reply to comment #56)
> (In reply to comment #55)
> > Things to test/fix:
> > * parent stacking context is "paints into ancestor"
> > * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> > * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.
> 
> Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?

Depends what "some tricks" means :)

> > If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?
> 
> Yes. It should only blend with the stacking context ancestor.

Authors are not going to like this.

[Dean Jackson]

Created attachment 214184 [details]
Patch

[Dean Jackson]

(In reply to comment #55)
> * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context

New patch has a test that shows we fail this.

> If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?

According to www-style, the answer is yes. Or more precisely, it's still blending, just with nothing.

[WebKit Commit Bot]

Attachment 214184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/effect-blend-mode-expected.txt', u'LayoutTests/css3/compositing/effect-blend-mode.html', u'LayoutTests/css3/compositing/reflected-blend-mode-expected.txt', u'LayoutTests/css3/compositing/reflected-blend-mode.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/should-not-have-compositing-layer.html', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png', u'LayoutTests/platform/mac/css3/compositing/reflected-blend-mode-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h']" exit_code: 1
Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

(In reply to comment #58)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > Things to test/fix:
> > > * parent stacking context is "paints into ancestor"
> > > * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> > > * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.
> > 
> > Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?
> 
> Depends what "some tricks" means :)
> 
> > > If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?
> > 
> > Yes. It should only blend with the stacking context ancestor.
> 
> Authors are not going to like this.

I agree that this will generate some surprises but I'm pretty sure that they will get used to it, especially if the developer tools keep improving (so authors can see the stacking contexts).

Can you see a way to make non-isolated grouping workable?

[Mihai Tica]

Created attachment 214448 [details]
Test case with preserve-3d

Blending currently fails when setting preserve-3d on an element.

[Mihai Tica]

Created attachment 214449 [details]
Screenshot of blending with preserve-3d

[Mihai Tica]

I've tested the patch with several accelerated elements such as canvas, video, plugin, 3d transform or iframe. Besides preserve-3d, in all other cases blending was performed.

[Simon Fraser (smfr)]

Blending needs to force transform-style: flat like filters do.

What I'd like to see is a solution for:

<div style="position:absolute; z-index: 0;">
   <div style="overflow:hidden">
      <div style="blend-mode: difference; transform: translateZ(0)"></div>
   </div>
</div>

[Mihai Tica]

Ok, I'll have have a look at this problem
(In reply to comment #66)
> Blending needs to force transform-style: flat like filters do.
> 
> What I'd like to see is a solution for:
> 
> <div style="position:absolute; z-index: 0;">
>    <div style="overflow:hidden">
>       <div style="blend-mode: difference; transform: translateZ(0)"></div>
>    </div>
> </div>

[Mihai Tica]

Created attachment 214735 [details]
Patch

[Mihai Tica]

(In reply to comment #68)
> Created an attachment (id=214735) [details]
> Patch

I've added a solution to the overflow:hidden problem with this patch, could you please have a look?

[Simon Fraser (smfr)]

(In reply to comment #69)
> (In reply to comment #68)
> > Created an attachment (id=214735) [details] [details]
> > Patch
> 
> I've added a solution to the overflow:hidden problem with this patch, could you please have a look?

Could you explain it?

[Mihai Tica]

(In reply to comment #70)
> (In reply to comment #69)
> > (In reply to comment #68)
> > > Created an attachment (id=214735) [details] [details] [details]
> > > Patch
> > 
> > I've added a solution to the overflow:hidden problem with this patch, could you please have a look?
> 
> Could you explain it?

When having a parent with "overflow:hidden", an ancestor clipping layer is additionally created for the element. When detecting the presence of this layer, I set the blendMode for this layer, causing the filter operation to be executed with the underlying content, which in our case is the backdrop needed for blending.

The change can be found in Source/WebCore/rendering/RenderLayerBacking.cpp:379

[Mihai Tica]

The previous patch didn't provide functionality for accelerated parents with overflow:hidden, for example:

<div class="parent" style="overflow:hidden; -webkit-transform: translateZ(0)">
    <div class="blendedChild" style="-webkit-blend-mode: difference"></div>
    <div class="nonBlendedChild"></div>
</div>

For this particular case, the parent element creates a clipping layer (m_childContainmentLayer), meaning that the blended child won't get an m_ancestorClippingLayer, breaking the previous functionality.
Setting the blend mode on the parent clipping layer is wrong because it would cause all of the parent's children to blend, instead of just the ones that have blending set.

To address this problem, I've found the following solution, which I've managed to validate with a prototype:

When detecting an element that should create a clipping layer, while also having blended children, we disallow the clipping layer creation and mark the element.

For each child layer, we check whether it has a blend mode or a subtree with a blended child.
If so, we create an ancestor clipping layer (m_ancestorClippingLayer) and set its size/location according the what the clipping layer for the parent should have been.
Otherwise, we have detected a child that doesn't have blending (neither the current element, nor its children), so we create a clipping layer.

When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.

Would this approach be acceptable? Are there any other things that we missed or that we should take into account? Do you see any other viable options?

[Simon Fraser (smfr)]

Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?

[Rik Cabanier]

(In reply to comment #73)
> Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?

If you put a filter on the element with clipping, that element becomes a stacking context.
This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.

[Simon Fraser (smfr)]

(In reply to comment #74)
> (In reply to comment #73)
> > Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?
> 
> If you put a filter on the element with clipping, that element becomes a stacking context.
> This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.

 I think you misunderstood. I was referring to:

> When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
> If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.

So you put the CAFilter on the ancestor clipping layer (which has -masksToBounds). Does this combination of masking and filtering give the correct result with blur?

[Mihai Tica]

(In reply to comment #75)
> (In reply to comment #74)
> > (In reply to comment #73)
> > > Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?
> > 
> > If you put a filter on the element with clipping, that element becomes a stacking context.
> > This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.
> 
>  I think you misunderstood. I was referring to:
> 
> > When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
> > If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.
> 
> So you put the CAFilter on the ancestor clipping layer (which has -masksToBounds). Does this combination of masking and filtering give the correct result with blur?

I'm not sure if blur is the best example here since it's not used as a filter for any blending operation, so it won't be set on the ancestor clipping layer.

Suppose an element has a mask, blending, and creates a clipping layer according to the model in #72.
In this case, the content would draw, masking would then be performed, followed in the end by the compositing filter specified by the blend mode.

If this element also has a filter, such as blur, the content is first drawn and then blurred. Next, the mask is applied and the results gets blended with the underlying content.
This behavior is stated as correct in the blending and compositing spec (see http://dev.w3.org/fxtf/compositing-1/#compositingandblendingorder), so I don't think this might be a problem.

[Mihai Tica]

Is this approach acceptable or should we investigate an alternative?
Simon, Dean, could you please comment on this?

[Simon Fraser (smfr)]

My main concern here is that we'll be hampered from making future changes to the CALayer tree by constraints imposed by blending; we might be able to special-case layers for overflow, .but in general, any CALayer tree config change that results in the blending layer not having the stacking context layers as its direct parent will break the rendering.

[Mihai Tica]

I did an investigation to find use cases similar to overflow:hidden and haven't found any additional ones.
I checked for accelerated elements that don't create a stacking context (found these in the list from WebCore/rendering/CompositionReasons.h) and parented them to an element that creates a stacking context, while also adding a blended child element. In all of these cases, the blending operation is performed with the topmost element (the stacking context), as stated in the spec.

Since we haven't found any additional issues, would it perhaps be acceptable to starting work on this?

[Mihai Tica]

Any updates? Could you please comment on this?

[Simon Fraser (smfr)]

Comment on attachment 214735 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: css3/compositing/blend-mode-layers.html

This needs some text summarizing the changes in the patch.

> Source/WebCore/rendering/RenderLayerBacking.cpp:384
> +    if (hasAncestorClippingLayer())
> +        ancestorClippingLayer()->setBlendMode(style->blendMode());
> +
> +    m_graphicsLayer->setBlendMode(style->blendMode());

Do you really want to set it on both here?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1061
> +        // If the layer composited for other reasons than blending, it is no longer needed to keep track if a child was blended

"keep track of whether a child"

> Source/WebCore/rendering/RenderLayerCompositor.h:85
> +    CompositingReasonRoot                                   = 1 << 23,
> +    CompositingReasonBlending                               = 1 << 24

The web inspector exposes these to authors. You should file a follow-up to ensure that the inspector shows blending as a layer creation reason.

[Mihai Tica]

Created attachment 219908 [details]
Patch

[WebKit Commit Bot]

Attachment 219908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Tica]

Created attachment 219911 [details]
Patch for review

[WebKit Commit Bot]

Attachment 219911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Tica]

Please take another look

[Dirk Schulze]

Comment on attachment 219911 [details]
Patch for review

rs=me

[WebKit Commit Bot]

Comment on attachment 219911 [details]
Patch for review

Rejecting attachment 219911 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 219911, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
c8f
r161556 = cb8504d4c7ab896253bbf8c7e1eb58ef5c4634c2
r161557 = 4a2c6f6ea09968b0ab42260c19fee2966b2eb915
r161558 = 8fb8a5c7a2a39884cf5dc9a050fc498578c62114
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
ERROR: LayoutTests/platform/efl/TestExpectations:499:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 1 files

Full output: http://webkit-queues.appspot.com/results/5068903893958656

[Joseph Pecoraro]

Comment on attachment 219911 [details]
Patch for review

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

> Source/WebCore/rendering/RenderLayerCompositor.h:86
> +    CompositingReasonBlending                               = 1 << 24

We should have the inspector list this reason:

  - Source/WebCore/inspector/prototype/LayerTree.json => update CompositingReasons to include "blending"
  - Source/WebCore/inspector/InspectorLayerTreeAgent.cpp => update InspectorLayerTreeAgent::reasonsForCompositingLayer to set blending
  - Source/WebInspectorUI/UserInterface/LayerTreeSidebarPanel.js => update _populateListOfCompositingReasons to add a user readable string when blending is a reason

Seeing as this landed I'll whip up an inspector fix:
<https://webkit.org/b/126706> Web Inspector: New Reason for Layers: CompositingReasonBlending

[Mihai Tica]

Created attachment 220819 [details]
Patch

[WebKit Commit Bot]

Attachment 220819 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 220819 [details]
Patch

Clearing flags on attachment: 220819

Committed r161628: <http://trac.webkit.org/changeset/161628>

[Brent Fulgham]

Comment on attachment 214184 [details]
Patch

Clearing review request flag since this is closed.