Bug 69747 – FEComponentTransfer element doesn't support dynamic invalidation

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

[Renata Hodovan]

FEComponentTransfer element doesn't support dynamic invalidation

[Renata Hodovan]

Created attachment 110532 [details]
Proposed patch

[Renata Hodovan]

Chromium ews is yellow 'cos the new tests need plaform specific expecteds on chromium. The rest failing tests don't contain any SVG elements, so I guess they are unrelated or flaky.

[Nikolas Zimmermann]

Comment on attachment 110532 [details]
Proposed patch

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

Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-)

r- because of abusing SVGElement to share code. Please rework this part, everything else is great.

> Source/WebCore/svg/SVGElement.cpp:141
> +        return;

Unnecessary.

> Source/WebCore/svg/SVGElement.h:120
> +    void invalidateParents();

This does not belong in SVGElement - it has a very generic name and leads to confusion. SVGElement should be kept as small as possible.
Why don't you introduce a free-function in eg. SVGFilterPrimitiveStandardAttributes.h or another filter-related class?

[Renata Hodovan]

Created attachment 110679 [details]
Propsed patch

> Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-)
Yeah, tests work manually fine. Perhaps you don't see it on the pngs, because the second pricture tries to approach the input image (e.g. the second parameter set contains the default values) and it's printed to the png.

Furthermore I renamed invalidateParents() to invalidateFilterPrimitiveParent() and moved to SVGFilterPrimitveStandardAttributes. As we talked on IRC it's declared as a free function.

[Renata Hodovan]

Created attachment 110680 [details]
Proposed patch

One unnecessary comment is removed.

[Nikolas Zimmermann]

Comment on attachment 110680 [details]
Proposed patch

Looks great, r=me.

[Nikolas Zimmermann]

Comment on attachment 110680 [details]
Proposed patch

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

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:174
> +{
> +    ContainerNode* parent = element->parentNode();

Ah forgot to say. You have to either add an early return if element can be null or add a ASSERT(element);

[Nikolas Zimmermann]

Comment on attachment 110680 [details]
Proposed patch

Clearing patch flag hoping that cr-linux EWS will stop processing the patch over and over.

[Nikolas Zimmermann]

Comment on attachment 110680 [details]
Proposed patch

Heh, obvious return missing in
+    if (attrName == SVGNames::inAttr)
+        invalidateFilterPrimitiveParent(this);
..
ASSERT_NOT_REACHED();

[Renata Hodovan]

Created attachment 110851 [details]
Propsed patch

Missing returns are added both in SVGComponentTransferFunction and SVGFEMergeNodeElement.
FIXME about the missing svgAttrChanged() is also removed from SVGComponentTransferFunction.

[Nikolas Zimmermann]

Comment on attachment 110851 [details]
Propsed patch

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

r=me, please fix this before landing:

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:144
> +    if (attrName == SVGNames::typeAttr
> +        || attrName == SVGNames::tableValuesAttr
> +        || attrName == SVGNames::slopeAttr
> +        || attrName == SVGNames::interceptAttr
> +        || attrName == SVGNames::amplitudeAttr
> +        || attrName == SVGNames::exponentAttr
> +        || attrName == SVGNames::offsetAttr) {

This is useless, isSupportedAttribute() checks the same, if that returned true, it's one of these attributes :-).

> Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:149
> +        invalidateFilterPrimitiveParent(this);
> +        return;
> +    }
> +
> +    ASSERT_NOT_REACHED();

Just make this: invalidateFilterPrimitiveParent(this);

[Tim Horton]

It looks to me like this was landed in http://trac.webkit.org/changeset/97369 and can be closed, no?