[webkit-reviews] review denied: [Bug 58531] Invalid color handling is broken for SVG : [Attachment 94926] updated patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Oliver Varga
<Varga.Oliver at stud.u-szeged.hu>'s request for review:
Bug 58531: Invalid color handling is broken for SVG
https://bugs.webkit.org/show_bug.cgi?id=58531

Attachment 94926: updated patch
https://bugs.webkit.org/attachment.cgi?id=94926&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94926&action=review

Okay, it turns out I have even more comments, we're discussing them on IRC soon
:-)

> LayoutTests/ChangeLog:8
> +	   Handling invalid colors in SVG. Fixed color inheritance (fill,
stroke, uri) .
> +	   Added png-s and expected files.
> +	   https://bugs.webkit.org/show_bug.cgi?id=58531
> +

Oops, I overlooked this probably, the style rules dictates:
<Bug title>
<Bug url>
\n
Your description.

It should read like:
Invalid color handling is broken for SVG
https://bugs.webkit.org/show_bug.cgi?id=58531

Update baselines for the tests covering invalid color handling.

> Source/WebCore/ChangeLog:7
> +	   Handling invalid colors in SVG. Fixed color inheritance (fill,
stroke, uri) .
> +	   Added png-s and expected files.
> +	   https://bugs.webkit.org/show_bug.cgi?id=58531

Invalid color handling is broken for SVG
https://bugs.webkit.org/show_bug.cgi?id=58531

Fix invalid color fallback handling. If the fill/stroke attributes computed
value leads to a an invalid color, inherit the desired color from the parent
style instead. Matches Opera/FF and SVG 1.1 Second Edition (Can you try to look
up a link to the spec here, and include it?)

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:88
>      RenderSVGResourceSolidColor* colorResource =
RenderSVGResource::sharedSolidPaintingResource();

Please introduce a helper function above this function:
static inline bool inheritColorFromParentStyleIfNeeded(RenderObject* object,
bool applyToFill, Color& color)
{
    if (color.isValid())
	return true;
    if (!object->parent() || !object->parent()->style())
	return false;
    const SVGRenderStyle* parentSVGStyle =
object->parent()->style()->svgStyle();
    color = applyToFill ? parentSVGStyle->fillPaintColor() :
parentSVGStyle->strokePaintColor();
}

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:89
>      if (paintType < SVGPaint::SVG_PAINTTYPE_URI_NONE) {

Comment is invalid, and can be avoided at all, as the
"inheritColorFromParentStyleIfNeeded" function is descriptive now...

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:-92
> -	   if (!color.isValid())
> -	       return 0;

Instead if:
if (!color.isValid())
    return 0;
use
if (!inheritColorFromParentStyleIfNeeded(object, applyToFill, color))
    return 0;

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:105
>	   // If a paint server is specified, and no or an invalid fallback
color is given, default to fill/stroke="black".

The comment is invalid now, remove it.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:111
> -	   if (!color.isValid())
> -	       color = Color::black;
> +	   if (!color.isValid()) {
> +	       if (!object->parent() || !object->parent()->style())
> +		   return 0;
> +	      const SVGRenderStyle* parentSVGStyle =
object->parent()->style()->svgStyle();
> +	      color = applyToFill ? parentSVGStyle->fillPaintColor() :
parentSVGStyle->strokePaintColor();
> +	   }

Use the new helper function.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:120
>	   // If a paint server is specified, and no or an invalid fallback
color is given, default to fill/stroke="black".

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:126
> +	   if (!color.isValid()) {
> +	       if (!object->parent() || !object->parent()->style())
> +		   return 0;
> +	      const SVGRenderStyle* parentSVGStyle =
object->parent()->style()->svgStyle();
> +	      color = applyToFill ? parentSVGStyle->fillPaintColor() :
parentSVGStyle->strokePaintColor();
> +	   }

Ditto.