[webkit-reviews] review denied: [Bug 43954] An individual renderer should be assigned to each SVGFE*Element class : [Attachment 66482] Draft patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 43954: An individual renderer should be assigned to each SVGFE*Element
class
https://bugs.webkit.org/show_bug.cgi?id=43954

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

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

Realy great patch! Some notes:

> WebCore/rendering/RenderSVGFilterPrimitive.cpp:37
> +namespace WebCore {
> +
> +
> +
> +} // namespace WebCore
> +
Ok, this file is useless at the moment. It should be added once the logic gets
added.

> WebCore/rendering/RenderSVGFilterPrimitive.h:37
> +class RenderSVGFilterPrimitive : public RenderSVGHiddenContainer {
We may should call it RenderSVGResourceFilterPrimitive, because it needs to be
controlled by RenderSVGResourceFilter later.

> WebCore/rendering/RenderSVGFilterPrimitive.h:41
> +	RenderSVGFilterPrimitive(SVGStyledElement* filterElement, FilterEffect*
filterEffect)
> +	    : RenderSVGHiddenContainer(filterElement)
> +	    , m_filterEffect(filterEffect)
Why don't you use SVGFilterPrimitiveStandardAttributes* primitiveElement here?
I also wouldn't set the FilterEffect here.

> WebCore/rendering/RenderSVGFilterPrimitive.h:46
> +	RefPtr<FilterEffect> m_filterEffect;
We don't set the RefPtr with your patch, I would let it out here. We have to
specify some kind of struct anyway here (see RenderSVGResource*.h). One
SVGFE*Element can be used by different targetElements, because the root
filterElement can be used by different targetElements. That means we have to
store all values depending on the targetElement.

> WebCore/svg/SVGFELightElement.cpp:82
> +	   if (parent() && parent()->renderer())
> +	      
RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent()->rendere
r());
This needs of course more testing. Is the parent realy a filterEffect? Don't
call renderer() multiple times, store the pointer in a variable.

> WebCore/svg/SVGFELightElement.cpp:131
> +    if (!changedByParser && parent() && parent()->renderer())
> +	  
RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent()->rendere
r());
dito

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:142
> +    return new (arena) RenderSVGFilterPrimitive(this, 0);
talked about the CTor some lines before.

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:55
> +	   if (renderer())
> +	      
RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer());
if (RenderObject* renderer = renderer())

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:61
> +    virtual bool rendererIsNeeded(RenderStyle*) { return true; }
This line is not neaded. true is the default.

An additional note. We should think about naming everything filterPrimitive
instead of filterEffect to be compatible with the spec. This is more a note for
me than an advice for you.

Again, the patch looks realy great. I wounder and am happy that this patch
fixes tests instead of breaking tests. Did you run all SVG-tests? Great job so
far.
(Reviewed the patch earlier, because the win-bots seem to take ages :-()