[webkit-reviews] review denied: [Bug 56906] CSS related SVG*Element changes doesn't require relayout : [Attachment 86946] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 56906: CSS related SVG*Element changes doesn't require relayout
https://bugs.webkit.org/show_bug.cgi?id=56906

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86946&action=review

> Source/WebCore/ChangeLog:8
> +	   The changes of some CSS related SVGFilter properties e.g.
lighting-color, flood_color, flood_opacity

s/_/-/

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:92
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom))

Isn't it enough to check if localName() is a nullatom? Also, don't we have
something like a nullQualifiedName? Some kind of static variable?

> Source/WebCore/svg/SVGFEFloodElement.cpp:46
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom)) {

Ditto.

> Source/WebCore/svg/SVGFESpecularLightingElement.cpp:97
> +    if (attrName == QualifiedName(nullAtom, nullAtom, nullAtom))

Ditto.

> LayoutTests/ChangeLog:9
> +	   Testing inherited CSS related SVG property changes by FEFlood,
FESpecularLighting and FEDiffuseLighing filters.
> +	   Adding a missing test to check the dynamic update of lighting-color
property of FESpecularLighting.

Can you describe why the pixel tests don't pass?
Also SVGFESpecularLightingElement-inherit-lighting-color-css-prop-expected.png
is completely whit. More confusing :-)