Avoid updateFromElement() usage in SVG

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Avoid updateFromElement() usage in SVG.

[Nikolas Zimmermann]

Created attachment 144195 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 144195 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:-119
> -void RenderSVGModelObject::updateFromElement()
> -{
> -    RenderObject::updateFromElement();
> -    SVGResourcesCache::clientUpdatedFromElement(this, style());
> -}
> -

Don't we need to add calls to clientWasAddedToTree and clientWillBeRemovedFromTree here?
RenderSVGShape, RenderSVGImage, etc... directly inherit from RenderSVGModelObject.

[Nikolas Zimmermann]

Created attachment 144197 [details]
Patch v2

[Nikolas Zimmermann]

(In reply to comment #2)
> Don't we need to add calls to clientWasAddedToTree and clientWillBeRemovedFromTree here?
> RenderSVGShape, RenderSVGImage, etc... directly inherit from RenderSVGModelObject.
As discussed on IRC, RenderSVGModelObject doesn't have children, so it doesn't need this logic.

[Ryosuke Niwa]

Comment on attachment 144195 [details]
Patch

I think this refactoring is done right (had a lengthy discussion with WildFox on IRC)., and new approach makes the removal and addition of SVG resource much more apparent when constructing or destructing the render tree.

[Ryosuke Niwa]

Comment on attachment 144197 [details]
Patch v2

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

> Source/WebCore/rendering/svg/SVGResourcesCache.cpp:140
> -    if (diff == StyleDifferenceEqual)
> +    if (diff == StyleDifferenceEqual || !renderer->parent())
>          return;
>  
>      // In this case the proper SVGFE*Element will decide whether the modified CSS properties require a relayout or repaint.
>      if (renderer->isSVGResourceFilterPrimitive() && diff == StyleDifferenceRepaint)
>          return;
>  
> -    clientUpdatedFromElement(renderer, newStyle);
> +    SVGResourcesCache* cache = resourcesCacheFromRenderObject(renderer);
> +    cache->removeResourcesFromRenderObject(renderer);
> +    cache->addResourcesFromRenderObject(renderer, newStyle);

I think this function deserves a little clarifying why we would want to remove/add the resource here.

[Nikolas Zimmermann]

(In reply to comment #5)
> (From update of attachment 144195 [details])
> I think this refactoring is done right (had a lengthy discussion with WildFox on IRC)., and new approach makes the removal and addition of SVG resource much more apparent when constructing or destructing the render tree.

Thanks! The discussion was very helpful. I left a new comment there:
    // Dynamic changes of CSS properties like 'clip-path' may require us to recompute the associated resources for a renderer.
    // FIXME: Avoid passing in a useless StyleDifference, but instead compare oldStyle/newStyle to see which resources changed
    // to be able to selectively rebuild individual resources, instead of all of them.

[Nikolas Zimmermann]

Committed r118608: <http://trac.webkit.org/changeset/118608>