Bug 95258 – Add support for blendmode to webkit rendering engine

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rik Cabanier]

This patch will add support for blendmode to the WebCore engine.
A later patch will add support for CoreAnimation and Chromium

[Rik Cabanier]

Created attachment 161090 [details]
First try

[Rik Cabanier]

No tests since the rendering and parsing didn't change

[Simon Fraser (smfr)]

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.

[Rik Cabanier]

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.

[Rik Cabanier]

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

[Simon Fraser (smfr)]

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?

[Rik Cabanier]

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

[Simon Fraser (smfr)]

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

[Rik Cabanier]

(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

[Rik Cabanier]

Created attachment 161330 [details]
renamed function so it's shorter

[Simon Fraser (smfr)]

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?

[Rik Cabanier]

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

[Rik Cabanier]

Created attachment 161359 [details]
updated all places where requiresLayer checked. Added comment + moved function to public declaration. Changed compositor so it call public function

[Simon Fraser (smfr)]

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.

[Rik Cabanier]

Created attachment 161391 [details]
changed the test file so it uses a <pre> instead of a <div>

[WebKit Review Bot]

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.

[e-mail] 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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.