[webkit-reviews] review denied: [Bug 52200] Small filter primitive renderer improvements : [Attachment 78684] 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 52200: Small filter primitive renderer improvements
https://bugs.webkit.org/show_bug.cgi?id=52200

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

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

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:334
> +void RenderSVGResourceFilter::repaintFilterPrimitve(RenderObject* object)
> +{
> +    RenderObject* filter = object->parent();
> +    if (!filter || !filter->isSVGResourceFilter())
> +	   return;
> +    filter->repaint();
> +}
> +

We'll definitely need this function later. But the policy wants us not to
commit dead code. And it is not in use atm.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:43
> +    SVGFilter* filter = static_cast<SVGFilter*>(effect->filter());

Please add an assert for effect->filter() before this line.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:76
> +    RefPtr<FilterEffect> effectReference = passEffectReference;
>      ASSERT(!m_effectReferences.contains(effectReference));
> +    ASSERT(object && !m_effectRenderer.contains(object));

You're right, a functions should always take the ownership, but IIRC you don't
touch passEffectReference in this function, but just save it in
m_effectRenderer. So do we need to store it in effectReference? Can't we just
store it m_effectRenderer? Also, do all callers of appendEffectToEffectRef...()
release the passed effect? - Ah you store RefPtr in two member variables,
m_effectReferences and m_effectRenderer. Hm, not sure what behavior is correct
here.