Bug 47181 – SVGFEMergeNodeElement doesn't support dynamic invalidation

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

SVGFEMergeNodeElement doesn't support dynamic invalidation

[Renata Hodovan]

Created attachment 69792 [details]
Patch for feMergeNode properties

[Dirk Schulze]

Comment on attachment 69792 [details]
Patch for feMergeNode properties

Not sure, why you need feComposite on the tests. A test like this should be as simple as possible :-) Nevertheless, the patch itself looks great, but you should describe in the ChangeLog what you did to fix this issue. For example why did you ask the parent? why doesn't feMergeNode have a renderer, so that you have to call invalidation via the parent? And so on. As long as you don't have commit privileges, you have to upload a second patch. :-( r- because of the ChangeLog.

[Renata Hodovan]

Created attachment 69913 [details]
Patch for feMergeNode properties

[Nikolas Zimmermann]

Comment on attachment 69913 [details]
Patch for feMergeNode properties

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

Looks good other than that, though as you can't commit yet on your own, you probably need to upload a new patch, hence r-.

> WebCore/ChangeLog:8
> +        Since feMergeNode doesn't have own renderer, we have to call the invalidation via it's parent.

s/it's/its/

> WebCore/svg/SVGFEMergeNodeElement.cpp:55
> +    if (attrName == SVGNames::inAttr) {

If there's only one attribute to consider, you may want to use early exit style here.
if (attrName != SVGNames::inAttr)
    return;

[Renata Hodovan]

Created attachment 69921 [details]
Patch for feMergeNode properties

[Nikolas Zimmermann]

Comment on attachment 69921 [details]
Patch for feMergeNode properties

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

Other than that it looks good...

> WebCore/svg/SVGFEMergeNodeElement.cpp:62
> +    if (attrName != SVGNames::inAttr)
> +        return;
> +    if (Node* parentNode = parent()) {
> +        RenderObject* renderer = parentNode->renderer();
> +        if (renderer && renderer->isSVGResourceFilterPrimitive())
> +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer);
> +    }
> +}

Sorry, for not being clear. I meant:

if (attrName != SVGNames::inAttr)
    return;

Node* parentNode = parent();
if (!parentNode)
    return;

RenderObject* renderer = parentNode->renderer();
if (!renderer || !renderer->isSVGResourceFilterPrimitive())
    return;

RenderSVGResource::mark...

[Renata Hodovan]

Created attachment 69927 [details]
Patch for feMergeNode properties

[Nikolas Zimmermann]

Comment on attachment 69927 [details]
Patch for feMergeNode properties

Looks good, r=me.

[WebKit Commit Bot]

Comment on attachment 69927 [details]
Patch for feMergeNode properties

Clearing flags on attachment: 69927

Committed r69206: <http://trac.webkit.org/changeset/69206>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.