[webkit-reviews] review denied: [Bug 110427] Add support for CSS blending to SVG : [Attachment 224365] Patch V4

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Mihai Tica <mitica at adobe.com>'s
request for review:
Bug 110427: Add support for CSS blending to SVG
https://bugs.webkit.org/show_bug.cgi?id=110427

Attachment 224365: Patch V4
https://bugs.webkit.org/attachment.cgi?id=224365&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.