Bug 110427 – Add support for CSS blending to SVG

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rik Cabanier]

The blending and compositing spec defines a keyword that allows blending: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#mix-blend-mode

This bug will add the support code that forwards the parsed CSS value to the SVG rendering code.

[Rik Cabanier]

Created attachment 189448 [details]
not for review. Just to see if everything still builds/tests

[WebKit Review Bot]

Comment on attachment 189448 [details]
not for review. Just to see if everything still builds/tests

Attachment 189448 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16656713

[WebKit Review Bot]

Comment on attachment 189448 [details]
not for review. Just to see if everything still builds/tests

Attachment 189448 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16653718

[EFL EWS Bot]

Comment on attachment 189448 [details]
not for review. Just to see if everything still builds/tests

Attachment 189448 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16648760

[Rik Cabanier]

Created attachment 189451 [details]
not for review. Just to see if everything still builds/tests

[Dirk Schulze]

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

The patch looks sane and I am really excited about seeing blending in SVG!

And yet, I still object to implementing this feature at this point for the following reason:

We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.

I will not support adding a new feature to the SVG code that we can not guarantee to work after this move. It would be extremely disappointing to web authors to loose this feature again.

I ask you to implement blending for RenderLayers first and proof that it works there (on HTML). Once these steps are done, I am open to add this functionality to SVGRenderingContext, if we didn't move to the RenderLayer code already at this point.

[Rik Cabanier]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=189451) [details] [details]
> > Patch
> 
> The patch looks sane and I am really excited about seeing blending in SVG!
> 
> And yet, I still object to implementing this feature at this point for the following reason:
> 
> We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> 
> I will not support adding a new feature to the SVG code that we can not guarantee to work after this move. It would be extremely disappointing to web authors to loose this feature again.
> 
> I ask you to implement blending for RenderLayers first and proof that it works there (on HTML). Once these steps are done, I am open to add this functionality to SVGRenderingContext, if we didn't move to the RenderLayer code already at this point.

There are 2 patches for blending:
https://bugs.webkit.org/show_bug.cgi?id=99200 for Core Animation
https://bugs.webkit.org/show_bug.cgi?id=95614 for Core Graphics

I have every intention at landing them. However, I didn't have time to finish them because they need a lot of follow up.

[Rik Cabanier]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=189451) [details] [details]
> > Patch
> 
> The patch looks sane and I am really excited about seeing blending in SVG!
> 
> And yet, I still object to implementing this feature at this point for the following reason:
> 
> We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> 
Do you know the timeline for this process? (months? years?)

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=189451) [details] [details] [details]
> > > Patch
> > 
> > The patch looks sane and I am really excited about seeing blending in SVG!
> > 
> > And yet, I still object to implementing this feature at this point for the following reason:
> > 
> > We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> > 
> Do you know the timeline for this process? (months? years?)

Definitely months, hopefully not years ;)

[Rik Cabanier]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Created an attachment (id=189451) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > The patch looks sane and I am really excited about seeing blending in SVG!
> > > 
> > > And yet, I still object to implementing this feature at this point for the following reason:
> > > 
> > > We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> > > 
> > Do you know the timeline for this process? (months? years?)
> 
> Definitely months, hopefully not years ;)

Since the changes to renderlayer should hopefully land before that, can this patch be submitted in the mean time?
It will also add test files so if svg makes the switch, those tests will fail and need to be fixed.

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > Created an attachment (id=189451) [details] [details] [details] [details] [details]
> > > > > Patch
> > > > 
> > > > The patch looks sane and I am really excited about seeing blending in SVG!
> > > > 
> > > > And yet, I still object to implementing this feature at this point for the following reason:
> > > > 
> > > > We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> > > > 
> > > Do you know the timeline for this process? (months? years?)
> > 
> > Definitely months, hopefully not years ;)
> 
> Since the changes to renderlayer should hopefully land before that, can this patch be submitted in the mean time?
> It will also add test files so if svg makes the switch, those tests will fail and need to be fixed.

This patch can wait till the patch for RenderLayer is ready, landed and proves that it works.

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #6)
> > > > > (In reply to comment #5)
> > > > > > Created an attachment (id=189451) [details] [details] [details] [details] [details] [details]
> > > > > > Patch
> > > > > 
> > > > > The patch looks sane and I am really excited about seeing blending in SVG!
> > > > > 
> > > > > And yet, I still object to implementing this feature at this point for the following reason:
> > > > > 
> > > > > We are in a transition process on moving SVG rendering and painting under RenderLayer, which allows us creating new layers in certain situations like opacity, (transforms), z-index and some other situations. At this time we will share a big chunk of the SVG code with HTML. This also means that we will have similar restrictions as HTML - SVGRenderinContext will mostly be gone.
> > > > > 
> > > > Do you know the timeline for this process? (months? years?)
> > > 
> > > Definitely months, hopefully not years ;)
> > 
> > Since the changes to renderlayer should hopefully land before that, can this patch be submitted in the mean time?
> > It will also add test files so if svg makes the switch, those tests will fail and need to be fixed.
> 
> This patch can wait till the patch for RenderLayer is ready, landed and proves that it works.

We discussed the issues on mailing lists and on IRC and in person with Dean and Simon. We might need more time to implement these things on HW accelerated layers, but we agreed on a general model to allow blending.
I want to see the 'clip-path' property (and the 'isolation' property later) to isolate so that we have a consistent behavior once we decide to HW accelerate SVG.
Once we have this implemented, it is of course harder to switch to HW for CA and CI and we should fallback to software in the case of blending. Once CALayers supports blending, we don't need this consideration.

To support isolation, we basically just need to call beginTransparencyLayer at the time of a clip-path. The patch should make sure that we just create a layer if blending is involved.

[Dirk Schulze]

(In reply to comment #12)
> To support isolation, we basically just need to call beginTransparencyLayer at the time of a clip-path. The patch should make sure that we just create a layer if blending is involved.

Forgot to mention that clip-path and -webkit-clip-path are supported in SVG and needs testing. Both need special isolation treatment in case of blending.

[Rik Cabanier]

Created attachment 205136 [details]
Patch

[Dirk Schulze]

Comment on attachment 205136 [details]
Patch

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

A great patch In general. I would like to have some changes on the way you test blend modes.

> Source/WebCore/ChangeLog:11
> +        This is the first patch to add support for mix-blend-mode for SVG.
> +        There will be subsequent patches to ensure interoperable behavior. For now,
> +        the Core Graphics behavior is considered the best but might need to be changed
> +        if we run into implementation issues.

Maybe you should open bug reports first and put FIXME's in the code base where we either expect wrong behavior or where we want to change the behavior. (Each FIXME should have a link to the bug report.) One of them would be the issue with masking/clipping. While CG does not isolate, all others isolate. This should be in the source code before calling mask/clip-path renderer.

> LayoutTests/ChangeLog:17
> +        * css3/compositing/resources/ducky_black.png: Added.
> +        * css3/compositing/svg-blend-mode-expected.txt: Added.
> +        * css3/compositing/svg-blend-mode-stacking-expected.txt: Added.
> +        * css3/compositing/svg-blend-mode-stacking.html: Added.
> +        * css3/compositing/svg-blend-mode.html: Added.
> +        * platform/mac/css3/compositing/svg-blend-mode-expected.png: Added.
> +        * platform/mac/css3/compositing/svg-blend-mode-stacking-expected.png: Added.

I would very much prefer ref tests instead of the pixel tests. Pixel tests can not be shared with other browser vendors and maybe platform dependent. Especially when you embed PNGs into a document. It should be fairly easy to generate ref tests instead. Use a gradient with sharp color jumps instead of smooth gradients (or rect angles with different colors that you blend over each other). It would even be more valuable if you do not try to test all at once, but create simple test cases where you just have one color as the resulting output (and can just be reproduced with exactly one blend mode for the given input).

You can also use these tests for the spec then, the second benefit of creating ref tests from the beginning. :)

In general, I would like to see the files split into MULTIPLE tests. This makes it easier to track down bugs in case of regressions.

> LayoutTests/css3/compositing/svg-blend-mode-expected.txt:1
> +

See test file.

> LayoutTests/css3/compositing/svg-blend-mode-stacking-expected.txt:2
> +T
> +

See test file

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

Why is that not an SVG file?

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:4
> +	if (window.testRunner)
> +	  window.testRunner.dumpAsText(true);

Looks like you don't really have somthing to dump. Please remove these lines. You should also remove the expected dump outputs.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:17
> +<style>
> +
> +li
> +{
> +	margin: 5px;
> +	width: 130px;
> +	height: 130px;
> +	background:  url("resources/ducky.png") no-repeat 0 0 /100% 100%, linear-gradient(to right, #00ffff 0%, rgba(0,0,255,0) 24%, #ff0000 50%, #ffff00 75%, #00ff00 100%);
> +	display: block;
> +	float: left;
> +}
> +</style>

You don't use <li> in this file. Remove it.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:19
> +<body style="background-color: green">

Can be replaced with a green rect drawn before you draw anything else.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:26
> +      <stop offset="0%" style="stop-color:rgb(0,0,255);stop-opacity:1" />
> +	  <stop offset="33%" style="stop-color:rgb(0,0,255);stop-opacity:0" />
> +	  <stop offset="66%" style="stop-color:rgb(0,0,255);stop-opacity:0" />
> +      <stop offset="100%" style="stop-color:rgb(0,0,255);stop-opacity:1" />

A blue gradient? Why do you just change alpha channel and not different colors? Even so, please use a gradient that can easily be used to create ref tests. Maybe don't use a gradient at all.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:29
> +	<image id="i" xlink:href="resources/ducky.png" style="-webkit-blend-mode: difference" x="0" y="0" height="130" width="130" preserveAspectRatio="none"/>

Please use something different than a PNG for testing blend modes. A simple colored rect for each blend mode test might be ok. Also, please use <use> less, so that it is easier to see on each individual test what happens.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:31
> +      <image xlink:href="resources/ducky_black.png" x="0" y="0" height="130" width="130" preserveAspectRatio="none"/>

Use a colored rectangle instead. Images in CG can influence color management in a bad way.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:44
> +      <feGaussianBlur in="SourceGraphic" stdDeviation="3" />

Maybe use feColorMatrix? Remember that filters are in linearRGB when creating ref tests.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:49
> +  <use xlink:href="#i" style="--webkit-blend-mode: difference"/>  

s/--webkit-blend-mode/-webkit-blend-mode/ same mistake everywhere.

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:60
> +  <g transform="translate(420, 0)" opacity=".99">
> +    <use xlink:href="#r"/>
> +    <use xlink:href="#i" style="--webkit-blend-mode: difference"/> 

opacity=0.99 is hard to test in ref tests. There might be rounding issues. Maybe you want to have a more stable value?

> LayoutTests/css3/compositing/svg-blend-mode-stacking.html:64
> +    <use xlink:href="#i" style="--webkit-blend-mode: difference" opacity=".99"/> 

opacity=0.99 is hard to test in ref tests. There might be rounding issues. Maybe you want to have a more stable value?

> LayoutTests/css3/compositing/svg-blend-mode.html:4
> +	if (window.testRunner)
> +	  window.testRunner.dumpAsText(true);

Ditto.

> LayoutTests/css3/compositing/svg-blend-mode.html:16
> +li
> +{
> +	margin: 5px;
> +	width: 130px;
> +	height: 130px;
> +	background:  url("resources/ducky.png") no-repeat 0 0 /100% 100%, linear-gradient(to right, #00ffff 0%, rgba(0,0,255,0) 24%, #ff0000 50%, #ffff00 75%, #00ff00 100%);
> +	display: block;
> +	float: left;
> +}

You don't use <li> in this file. Why is it an HTML file at all? Why not an SVG file?

[Mihai Tica]

Created attachment 223119 [details]
Patch

[Mihai Tica]

Created attachment 223216 [details]
Patch V2

[Dirk Schulze]

My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.

Do we already have the isolation property? I guess not. If we have this should isolate as well.

[Rik Cabanier]

(In reply to comment #18)
> My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> 
> Do we already have the isolation property? I guess not. If we have this should isolate as well.

Masking already isolates and clipping doesn't.
I guess we should add a test to verify this.

[Dirk Schulze]

(In reply to comment #19)
> (In reply to comment #18)
> > My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> > 
> > Do we already have the isolation property? I guess not. If we have this should isolate as well.
> 
> Masking already isolates and clipping doesn't.
> I guess we should add a test to verify this.

Are you sure that masking does? I think the last time we checked it didn't. And still, this doesn't answer the question. Should or shouldn't mask and clip-path isolate? IIRC according to the spec they should. Do we want this behavior?

And yes, tests with masking, clip-path and filters would be great. Same with opacity and svg-shadows (which are affected indirectly) as well.

Also, could you take a look into creating ref-tests Mihai?

[Rik Cabanier]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> > > 
> > > Do we already have the isolation property? I guess not. If we have this should isolate as well.
> > 
> > Masking already isolates and clipping doesn't.
> > I guess we should add a test to verify this.
> 
> Are you sure that masking does? I think the last time we checked it didn't. 

It did.

>And still, this doesn't answer the question. Should or shouldn't mask and clip-path isolate? IIRC according to the spec they should. Do we want this behavior?

The spec call this out: http://dev.w3.org/fxtf/compositing-1/#csscompositingrules_SVG
Clipping is not in the list. It was there originally but after experimenting in the various browsers, we took it out.

> 
> And yes, tests with masking, clip-path and filters would be great. Same with opacity and svg-shadows (which are affected indirectly) as well.
> 
> Also, could you take a look into creating ref-tests Mihai?

[Mihai Tica]

(In reply to comment #18)
> My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> 
> Do we already have the isolation property? I guess not. If we have this should isolate as well.

I'll have a look into this. The isolation property isn't yet implemented.

[Mihai Tica]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> > > 
> > > Do we already have the isolation property? I guess not. If we have this should isolate as well.
> > 
> > Masking already isolates and clipping doesn't.
> > I guess we should add a test to verify this.
> 
> Are you sure that masking does? I think the last time we checked it didn't. And still, this doesn't answer the question. Should or shouldn't mask and clip-path isolate? IIRC according to the spec they should. Do we want this behavior?
> 
> And yes, tests with masking, clip-path and filters would be great. Same with opacity and svg-shadows (which are affected indirectly) as well.
> 
> Also, could you take a look into creating ref-tests Mihai?

Initially, I wanted to add ref tests, however, the blending results are different across platforms. On my 10.8 machine, I ran a simple test:
Two rectangles, one drawn with #FF0 and one with #F00, blended with difference should yield #0F0. While on my 10.8 machine, this test would pass, on 10.9, I got a 0.6% difference. This is the reason why I added the suite as pixel tests.

I already conducted an investigation and it seems like CG does yield slightly different results between 10.8 and 10.9.

In order to possibly convert these to ref tests in a near future, I'm thinking of proposing a |testRunner.addTolerance| method for RWT. What do you think?

[Mihai Tica]

Comment on attachment 223216 [details]
Patch V2

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

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:125
>              m_paintInfo->context->beginTransparencyLayer(opacity);

(In reply to comment #18)
> My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> 
> Do we already have the isolation property? I guess not. If we have this should isolate as well.

Why are you saying this patch doesn't force isolation for blending? It does isolate it by creating a transparency layer at this very line. Am I missing something?

[Dirk Schulze]

(In reply to comment #24)
> (From update of attachment 223216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223216&action=review
> 
> > Source/WebCore/rendering/svg/SVGRenderingContext.cpp:125
> >              m_paintInfo->context->beginTransparencyLayer(opacity);
> 
> (In reply to comment #18)
> > My question to the patch is the following. Do we want to force isolation as the specification demands? This patch is not doing it. It just isolates for opacity and sag-shadow.
> > 
> > Do we already have the isolation property? I guess not. If we have this should isolate as well.
> 
> Why are you saying this patch doesn't force isolation for blending? It does isolate it by creating a transparency layer at this very line. Am I missing something?

I said that it isolates for opacity and svg-shadow. Yes, for both you create a transparency layer.

But, I do not see where we create a transparency layer for clip-path or masking. Rik removed clip-path from the properties. I know that other platforms isolate for masking by default. I think I remember that this was not the case for CG for some reason. This is why I would like to see a test case for that.

[Mihai Tica]

Created attachment 223690 [details]
Patch V3

Adding tests to check if isolation is performed. Besides testing each blend mode, this patch also validates that opacity, webkit-shadow, masks, filters and blending cause isolation. As the spec states, clip-path doesn't isolate. Note that while masking doesn't create a transparency layer, the contents of the masked element are drawn in a separate buffer, thus creating isolation.

[Mihai Tica]

Created attachment 224226 [details]
Don't commit: Mask isolation

This is a solution I've found so that masks can isolate blending. Could you please have a look and comment on this?

[Rik Cabanier]

(In reply to comment #27)
> Created an attachment (id=224226) [details]
> Don't commit: Mask isolation
> 
> This is a solution I've found so that masks can isolate blending. Could you please have a look and comment on this?

This looks like a reasonable approach to me. The alternative is to always create layers for masking which is clearly too expensive.
I'm a bit sad that we're disabling this clearly desirable behavior of CG but we can bring it back once we have non-isolated groups.

[Mihai Tica]

Created attachment 224365 [details]
Patch V4

This patch enables -webkit-blend-mode and also causes masked elements to isolate blending.

[Dirk Schulze]

Comment on attachment 224365 [details]
Patch V4

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

Some smaller snippets. Looks good otherwise.

I still don't like the extra code for isolation on mask. I don't see another way to do it without harming performance though :(.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:449
> +    if (renderer.element() && renderer.element()->isSVGElement())

The second condition can be an ASSERT I think.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:454
> +void SVGRenderSupport::updateMaskedAncestorShouldIsolateBlending(RenderElement& renderer)

This should be in the compiler flag as well to make indicate that it is for blending. Ditto in .h file.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:462
> +    ASSERT(renderer.element());
> +    if (!renderer.element())
> +        return;
> +
> +    ASSERT(renderer.element()->isSVGElement());
> +    if (!renderer.element()->isSVGElement())
> +        return;

Don't you check all that before calling the function? 
ASSERT(renderer.element());
ASSERT(renderer.element()->isSVGElement());

should be enough.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:465
> +    bool maskedAncestorShouldIsolateBlending = renderer.style().hasBlendMode();
> +    for (auto ancestor = renderer.element(); ancestor && ancestor->isSVGElement(); ancestor = ancestor->parentElement()) {

We already isolate in certain cases. opacity and shadows are two examples. Filters should as well. No need to go further up the tree.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:470
> +        SVGGraphicsElement* graphicsElement = toSVGGraphicsElement(ancestor);
> +        if (graphicsElement) {

This can be combined into one line:
if (SVGGraphicsElement* graphicsElement = toSVGGraphicsElement(ancestor))

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:113
> +        bool isSVGGraphicsElement = toSVGElement(renderer.element())->isSVGGraphicsElement();
> +
> +        if (isSVGGraphicsElement) {

Should be possible to do the same if (toSVGElement(renderer.element())->isSVGGraphicsElement()) You may combine it with the first if clause: && toSVGElement(renderer.element())->isSVGGraphicsElement())

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:114
> +            SVGGraphicsElement* graphicsElement = toSVGGraphicsElement(renderer.element());

A reference SVGGraphicsElement& instead of the pointer

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:122
> +        m_paintInfo->context->clip(repaintRect);

Why clipping here now? We just did for shadows before.

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Add some lines explaining what you are testing.

[Mihai Tica]

Comment on attachment 224365 [details]
Patch V4

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

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:449
>> +    if (renderer.element() && renderer.element()->isSVGElement())
> 
> The second condition can be an ASSERT I think.

Unfortunately no. This gets called from RenderElement::styleDidChange.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:454
>> +void SVGRenderSupport::updateMaskedAncestorShouldIsolateBlending(RenderElement& renderer)
> 
> This should be in the compiler flag as well to make indicate that it is for blending. Ditto in .h file.

Will do.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:462
>> +        return;
> 
> Don't you check all that before calling the function? 
> ASSERT(renderer.element());
> ASSERT(renderer.element()->isSVGElement());
> 
> should be enough.

Ok.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:465
>> +    for (auto ancestor = renderer.element(); ancestor && ancestor->isSVGElement(); ancestor = ancestor->parentElement()) {
> 
> We already isolate in certain cases. opacity and shadows are two examples. Filters should as well. No need to go further up the tree.

I'll stop when detecting these properties. Also, I will only traverse when I detect that blending is set or unset.

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:470
>> +        if (graphicsElement) {
> 
> This can be combined into one line:
> if (SVGGraphicsElement* graphicsElement = toSVGGraphicsElement(ancestor))

OK

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:113
>> +        if (isSVGGraphicsElement) {
> 
> Should be possible to do the same if (toSVGElement(renderer.element())->isSVGGraphicsElement()) You may combine it with the first if clause: && toSVGElement(renderer.element())->isSVGGraphicsElement())

OK

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:114
>> +            SVGGraphicsElement* graphicsElement = toSVGGraphicsElement(renderer.element());
> 
> A reference SVGGraphicsElement& instead of the pointer

OK

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:122
>> +        m_paintInfo->context->clip(repaintRect);
> 
> Why clipping here now? We just did for shadows before.

When having opacity and shadow, this got called twice. Is this necessary?

>> LayoutTests/ChangeLog:7
>> +
> 
> Add some lines explaining what you are testing.

OK

[Mihai Tica]

Created attachment 224512 [details]
Patch V5

Addressing Dirk's comments.

[Dirk Schulze]

Comment on attachment 224365 [details]
Patch V4

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

>>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:122
>>> +        m_paintInfo->context->clip(repaintRect);
>> 
>> Why clipping here now? We just did for shadows before.
> 
> When having opacity and shadow, this got called twice. Is this necessary?

Maybe this was a requirement for CGs transparency layer implementation that you had to restrict the area. Not sure anymore. I assume you run pixel tests on debug builds and didn't see any regressions nor CGAssertions?

[Dirk Schulze]

Comment on attachment 224512 [details]
Patch V5

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 224512 [details]
Patch V5

Clearing flags on attachment: 224512

Committed r164294: <http://trac.webkit.org/changeset/164294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.