Bug 130891

Created attachment 228032 [details] failing use case

Created attachment 228828 [details] patch #1

The patch #1 is based on the patch submitted for this bug: https://bugs.webkit.org/show_bug.cgi?id=130892

Comment on attachment 228828 [details] patch #1 View in context: https://bugs.webkit.org/attachment.cgi?id=228828&action=review > Source/WebCore/ChangeLog:11 > + The stacking context layers having unisolated blending descendants should be able to create I guess you are referring to stacking content layers in normal flow here. > Source/WebCore/ChangeLog:21 > + There is another affected flag for ancestors (A) of the isolating stacking context layer S: What is this comment about? > Source/WebCore/ChangeLog:29 > + (WebCore::RenderLayer::updateDescendantDependentFlags): turning off the isSelfPaintgLayer flag. isSelfPaintgLayer -> isSelfPaintingLayer > LayoutTests/ChangeLog:3 > + [CSS Blending] Blending doesn't work if the parent stacking context is a normal flow only element I would like to see tests in which this behaviour- transitioning between normal flow only and self painting layer - is dynamically tested. For the layout tests, please make sure you have the right indentation. > LayoutTests/css3/compositing/blend-mode-accelerated-parent-overflow-hidden-transparent.html:13 > + margin-left: 10px; Why do you need this? > LayoutTests/css3/compositing/blend-mode-accelerated-parent-overflow-hidden-transparent.html:26 > +<p>This test checks that accelerated blending has no effect when the backdrop is a transparent normal flow only layer.</p> Please add a line here in which you describe what success looks like when you run the test manually. For instance: <p>On success you should see a green rectangle below.</p> > LayoutTests/css3/compositing/blend-mode-parent-overflow-hidden.html:22 > +<p>This test checks that software blending has no effect when the backdrop is a transparent normal flow only layer.</p> Same as above, please add a note about how success looks like when you manually load this test into a browser.

Created attachment 230293 [details] Patch

Created attachment 230376 [details] Patch

Comment on attachment 230376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230376&action=review > Source/WebCore/rendering/RenderLayer.cpp:825 > + if (m_hasUnisolatedBlendingDescendantsStatusDirty) { Do an early return instead of having the whole body indented. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1108 > +#if ENABLE(CSS_COMPOSITING) > + layer.updateIsolationForBlendingDescendantsIfNeeded(); > +#endif > layer.updateDescendantDependentFlags(); Can layer.updateIsolationForBlendingDescendantsIfNeeded() be rolled into layer.updateDescendantDependentFlags()? That would avoid adding an extra child walk. > Source/WebCore/rendering/RenderLayerModelObject.cpp:157 > +#if ENABLE(CSS_COMPOSITING) > + if (oldStyle->hasBlendMode()) > + layer()->parent()->dirtyAncestorChainHasBlendingDescendants(); > +#endif Can't we do this in RenderLayer::styleChanged()? > LayoutTests/ChangeLog:19 > + * css3/compositing/blend-mode-isolation-accelerated-overflow-hidden-expected.txt: Added. > + * css3/compositing/blend-mode-isolation-accelerated-overflow-hidden.html: Added. > + * css3/compositing/blend-mode-isolation-overflow-hidden-expected.html: Added. > + * css3/compositing/blend-mode-isolation-overflow-hidden.html: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer-expected.txt: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer.html: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer1-expected.txt: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer1.html: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer2-expected.txt: Added. > + * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer2.html: Added. > + * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer-expected.txt: Added. > + * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer.html: Added. Can we make these ref tests please?

Created attachment 230417 [details] Patch

Comment on attachment 230376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230376&action=review >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1108 >> layer.updateDescendantDependentFlags(); > > Can layer.updateIsolationForBlendingDescendantsIfNeeded() be rolled into layer.updateDescendantDependentFlags()? That would avoid adding an extra child walk. Done. I had to change some code for the other flags, but not the logic. >> Source/WebCore/rendering/RenderLayerModelObject.cpp:157 >> +#endif > > Can't we do this in RenderLayer::styleChanged()? On this branch RenderLayer is going to be removed for this RenderObject and RenderLayer::styleChanged will never get called. >> LayoutTests/ChangeLog:19 >> + * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer.html: Added. > > Can we make these ref tests please? It's hard to make ref tests for blending. There is a bug regarding wrong color results: https://bugs.webkit.org/show_bug.cgi?id=130582 and we don't really know what colors to expect. The blending output colors have slightly diferrent on retina vs non retina displays and among OS versions. I think layer tree is the more appropriate way to test isolation, because isolation flag is also dumped in RenderTreeAsText.

Comment on attachment 230417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review > Source/WebCore/rendering/RenderLayer.cpp:1129 > break; We can optimize this descendant walk in updateDescendantDependentFlags by leaving the loop if just dirty flags are set, not all of them. Also, we can skip entering updateDescendantDependentFlags() on a child if some conditions are met. For example if we know that a child has blendMode, we are not interested if it has other descendants with blend modes; if a child is self painting layer, we are not interested if it has descendants with that property. I can address this in a follow up patch.

Comment on attachment 230417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review The final review should definitely be done by someone with more insight. In general, the patch looks good to me. Some snippets. > Source/WebCore/ChangeLog:27 > + When hasUnisolatedBlendingDescendants gets set we make sure that isSelfPaintingLayer flag is set too. I think it looks better to replace Unisolated with NotIsolated . Seems easier to read. > Source/WebCore/ChangeLog:34 > + This change should not affect the logics behind the other flags. I hope our test coverage is good enough to assume that. > Source/WebCore/ChangeLog:39 > + Changing isolatesBlending() to check isStackingContext() first, this will avoid > + the assertion in hasUnisolatedBlendingDescendants() for non-stacking context elements. Wow, there should probably a comment in RenderLayer.h to warn others. >> Source/WebCore/rendering/RenderLayer.cpp:1129 >> break; > > We can optimize this descendant walk in updateDescendantDependentFlags by leaving the loop if just dirty flags are set, not all of them. Also, we can skip entering updateDescendantDependentFlags() on a child if some conditions are met. For example if we know that a child has blendMode, we are not interested if it has other descendants with blend modes; if a child is self painting layer, we are not interested if it has descendants with that property. I can address this in a follow up patch. Can/Should it be done before this patch then? To be honest, I am not really into the code to judge. After reading though the code again, I think it is fine to do this in a second patch afterwards. > Source/WebCore/rendering/RenderLayer.h:805 > + bool isolatesBlending() const { return isStackingContext() && hasUnisolatedBlendingDescendants(); } Please put a comment why it is necessary to do that.

Comment on attachment 230417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review >> Source/WebCore/ChangeLog:27 >> + When hasUnisolatedBlendingDescendants gets set we make sure that isSelfPaintingLayer flag is set too. > > I think it looks better to replace Unisolated with NotIsolated . Seems easier to read. Ok. >> Source/WebCore/ChangeLog:39 >> + the assertion in hasUnisolatedBlendingDescendants() for non-stacking context elements. > > Wow, there should probably a comment in RenderLayer.h to warn others. Actually I think we should remove the ASSERT from hasUnisolatedBlendingDescendants() for now. I still hit it in some circumstances and I've heard other people complaining about that too. Asserts have been removed for the other flags as well (see comments in RenderLayer.h: https://bugs.webkit.org/show_bug.cgi?id=71044). I hit this assertion mostly on minibrowser and in layout tests. On safari flags are updated correctly after changing the style because computeCompositingRequirements calls updateDescendantDependentFlags. We need a generic mechanism for updating all types of dirty flags on ancestors after the style gets changed and turn the asserts back on. I will remove this change and I'll comment the assert, addressing the issue #71044 with another patch.

Created attachment 230903 [details] Patch

Comment on attachment 230903 [details] Patch "LGTM" - but please give smfr a chance to reject my review.

Comment on attachment 230903 [details] Patch Looks fine.

Comment on attachment 230903 [details] Patch Clearing flags on attachment: 230903 Committed r168462: <http://trac.webkit.org/changeset/168462>

All reviewed patches have been landed. Closing bug.