This patch will add support for blendmode to the WebCore engine. A later patch will add support for CoreAnimation and Chromium
Created attachment 161090 [details] First try
No tests since the rendering and parsing didn't change
Comment on attachment 161090 [details] First try View in context: https://bugs.webkit.org/attachment.cgi?id=161090&action=review > Source/WebCore/ChangeLog:12 > + No new tests (OOPS!). OOPS This patch does change behavior: it makes compositing layers for blending, so this should be tested. > Source/WebCore/rendering/RenderInline.h:128 > + virtual bool requiresLayer() const { return isInFlowPositioned() || isTransparent() || hasMask() || hasFilter() || hasBlendMode(); } It might be worth making a method on RenderObject for hasMask() || hasFilter() || hasBlendMode(), since these all have similar effects. > Source/WebCore/rendering/RenderLayerBacking.cpp:963 > { > - return style->hasBorder() || style->hasBorderRadius() || style->hasOutline() || style->hasAppearance() || style->boxShadow() || style->hasFilter(); > + return style->hasBorder() || style->hasBorderRadius() || style->hasOutline() || style->hasAppearance() || style->boxShadow() || style->hasFilter() || style->hasBlendMode(); > } I'm not sure this is correct. We use hasBoxDecorations() to tell if we need backing store, but I don't think the blend mode needs to affect that answer. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1484 > + || renderer->hasBlendMode()) Again, not sure if this is true for all platforms. I don't think we need backing store for CA to do blending.
Created attachment 161292 [details] added test that checks for compositing layer. Removed test from RenderLayerCompositor.cpp. Added helper function that tests for mask, filter and blend.
(In reply to comment #3) > (From update of attachment 161090 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161090&action=review > > > Source/WebCore/ChangeLog:12 > > + No new tests (OOPS!). > > OOPS > > This patch does change behavior: it makes compositing layers for blending, so this should be tested. I added a test to the patch > > > Source/WebCore/rendering/RenderInline.h:128 > > + virtual bool requiresLayer() const { return isInFlowPositioned() || isTransparent() || hasMask() || hasFilter() || hasBlendMode(); } > > It might be worth making a method on RenderObject for hasMask() || hasFilter() || hasBlendMode(), since these all have similar effects. I added a helper method to RenderObject. > > > Source/WebCore/rendering/RenderLayerBacking.cpp:963 > > { > > - return style->hasBorder() || style->hasBorderRadius() || style->hasOutline() || style->hasAppearance() || style->boxShadow() || style->hasFilter(); > > + return style->hasBorder() || style->hasBorderRadius() || style->hasOutline() || style->hasAppearance() || style->boxShadow() || style->hasFilter() || style->hasBlendMode(); > > } > > I'm not sure this is correct. We use hasBoxDecorations() to tell if we need backing store, but I don't think the blend mode needs to affect that answer. You were right. This wasn't needed. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1484 > > + || renderer->hasBlendMode()) > > Again, not sure if this is true for all platforms. I don't think we need backing store for CA to do blending. I verified that it was needed in this case. Blending is just like opacity so it should follow the same logic.
Comment on attachment 161292 [details] added test that checks for compositing layer. Removed test from RenderLayerCompositor.cpp. Added helper function that tests for mask, filter and blend. View in context: https://bugs.webkit.org/attachment.cgi?id=161292&action=review > Source/WebCore/rendering/RenderObject.h:943 > + bool hasEffectThatNeedsLayer() const { return hasMask() || hasFilter() || hasBlendMode(); } I think this name is a bit literal. Maybe hasGraphicalEffect() or something? Is there generic name in graphics for these treatments that require that descendants need to be pre-rendered?
(In reply to comment #6) > (From update of attachment 161292 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161292&action=review > > > Source/WebCore/rendering/RenderObject.h:943 > > + bool hasEffectThatNeedsLayer() const { return hasMask() || hasFilter() || hasBlendMode(); } > > I think this name is a bit literal. Maybe hasGraphicalEffect() or something? Is there generic name in graphics for these treatments that require that descendants need to be pre-rendered? That would be a 'group' (aka a layer) Should I rename it to graphicalEffectNeedsGroup()?
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 161292 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=161292&action=review > > > > > Source/WebCore/rendering/RenderObject.h:943 > > > + bool hasEffectThatNeedsLayer() const { return hasMask() || hasFilter() || hasBlendMode(); } > > > > I think this name is a bit literal. Maybe hasGraphicalEffect() or something? Is there generic name in graphics for these treatments that require that descendants need to be pre-rendered? > > That would be a 'group' (aka a layer) > Should I rename it to graphicalEffectNeedsGroup()? hasGroupEffect()? createsGroup()?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 161292 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=161292&action=review > > > > > > > Source/WebCore/rendering/RenderObject.h:943 > > > > + bool hasEffectThatNeedsLayer() const { return hasMask() || hasFilter() || hasBlendMode(); } > > > > > > I think this name is a bit literal. Maybe hasGraphicalEffect() or something? Is there generic name in graphics for these treatments that require that descendants need to be pre-rendered? > > > > That would be a 'group' (aka a layer) > > Should I rename it to graphicalEffectNeedsGroup()? > > hasGroupEffect()? > createsGroup()? createsGroup sounds good. needsGroup would be good too
Created attachment 161330 [details] renamed function so it's shorter
Comment on attachment 161330 [details] renamed function so it's shorter View in context: https://bugs.webkit.org/attachment.cgi?id=161330&action=review > Source/WebCore/rendering/RenderBoxModelObject.h:90 > + virtual bool requiresLayer() const { return isRoot() || isPositioned() || isTransparent() || hasTransform() || hasHiddenBackface() || hasReflection() || createsGroup() || style()->specifiesColumns(); } You also need to change RenderBox::requiresLayer() and RenderTableRow::requiresLayer(). > Source/WebCore/rendering/RenderLayerCompositor.cpp:1780 > + if (hasCompositedDescendants && (layer->transform() || renderer->isTransparent() || renderer->hasMask() || renderer->hasReflection() || renderer->hasFilter() || renderer->hasBlendMode())) { > reason = RenderLayer::IndirectCompositingForGraphicalEffect; Why not use createsGroup() here? > Source/WebCore/rendering/RenderObject.h:943 > + bool createsGroup() const { return hasMask() || hasFilter() || hasBlendMode(); } I think this deserves a comment explaining the usage of "group". Should isTransparent() also be included?
(In reply to comment #11) > (From update of attachment 161330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161330&action=review > > > Source/WebCore/rendering/RenderBoxModelObject.h:90 > > + virtual bool requiresLayer() const { return isRoot() || isPositioned() || isTransparent() || hasTransform() || hasHiddenBackface() || hasReflection() || createsGroup() || style()->specifiesColumns(); } > > You also need to change RenderBox::requiresLayer() and RenderTableRow::requiresLayer(). > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1780 > > + if (hasCompositedDescendants && (layer->transform() || renderer->isTransparent() || renderer->hasMask() || renderer->hasReflection() || renderer->hasFilter() || renderer->hasBlendMode())) { > > reason = RenderLayer::IndirectCompositingForGraphicalEffect; > > Why not use createsGroup() here? > > > Source/WebCore/rendering/RenderObject.h:943 > > + bool createsGroup() const { return hasMask() || hasFilter() || hasBlendMode(); } > > I think this deserves a comment explaining the usage of "group". Should isTransparent() also be included? yes, I will add that. hasTransform seems like it should be added as well, except that RenderInline does not check for that. Is it because inline is never transformed?
Created attachment 161359 [details] updated all places where requiresLayer checked. Added comment + moved function to public declaration. Changed compositor so it call public function
Comment on attachment 161359 [details] updated all places where requiresLayer checked. Added comment + moved function to public declaration. Changed compositor so it call public function View in context: https://bugs.webkit.org/attachment.cgi?id=161359&action=review > LayoutTests/css3/compositing/should-have-compositing-layer.html:30 > +<div id="layertree"></div> Best to use a <pre> for this so that indentation is maintained.
Created attachment 161391 [details] changed the test file so it uses a <pre> instead of a <div>
Comment on attachment 161391 [details] changed the test file so it uses a <pre> instead of a <div> Rejecting attachment 161391 [details] from commit-queue. cabanier@adobe.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 161391 [details] changed the test file so it uses a <pre> instead of a <div> Rejecting attachment 161391 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13684555
Comment on attachment 161391 [details] changed the test file so it uses a <pre> instead of a <div> Clearing flags on attachment: 161391 Committed r127162: <http://trac.webkit.org/changeset/127162>
All reviewed patches have been landed. Closing bug.