Created attachment 168530 [details] Patch

Created attachment 168532 [details] Patch

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

Created attachment 168536 [details] Patch

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.

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

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.

Created attachment 168574 [details] Patch

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

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.

(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)

Created attachment 168746 [details] Fixes per smfr

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.

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

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.

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

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

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.

Created attachment 168853 [details] set the mime type

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.

So much noise.....

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

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?

(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

Created attachment 169204 [details] incorporated Alex' comments

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.

Created attachment 169274 [details] Patch

Created attachment 169287 [details] removed retainptr

Created attachment 169308 [details] Patch

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

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.

Created attachment 174241 [details] Patch

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

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

Created attachment 174288 [details] Made testfile more clear

Created attachment 189576 [details] refreshed patch

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

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

Created attachment 200723 [details] Patch

Created attachment 203313 [details] Patch

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

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

Created attachment 203342 [details] Patch

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

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.

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;

(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

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

Created attachment 214027 [details] Patch

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.

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.

Created attachment 214040 [details] Screenshot of test case

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.

(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

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?

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

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

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

Created attachment 214184 [details] Patch

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

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.

(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?

Created attachment 214448 [details] Test case with preserve-3d Blending currently fails when setting preserve-3d on an element.

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

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.

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>

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>

Created attachment 214735 [details] Patch

(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?

(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?

(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

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?

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?

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

(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?

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

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

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.

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?

Any updates? Could you please comment on this?

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.

Created attachment 219908 [details] Patch

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.

Created attachment 219911 [details] Patch for review

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.

Please take another look

Comment on attachment 219911 [details] Patch for review rs=me

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

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

Created attachment 220819 [details] Patch

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.

Comment on attachment 220819 [details] Patch Clearing flags on attachment: 220819 Committed r161628: <http://trac.webkit.org/changeset/161628>

Comment on attachment 214184 [details] Patch Clearing review request flag since this is closed.