[Webkit-unassigned] [Bug 43954] An individual renderer should be assigned to each SVGFE*Element class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43954


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66482|review?                     |review-
               Flag|                            |




--- Comment #2 from Dirk Schulze <krit at webkit.org>  2010-09-03 06:37:59 PST ---
(From update of attachment 66482)
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()->renderer());
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()->renderer());
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 :-()

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.