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

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

I'd like to see Niko to look at this patch as well. Maybe he has some more
proposals.

PS: Why do you add cq flag? You have commit rights and in my opinion it is
better to land a patch yourself, you have to be available on landing anyway to
fix problems.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:114
> +	   builder->appendEffectToEffectReferences(effect,
effectElement->renderer());

effect needs to be release if you change RefPtr to PassRefPtr in
appendEffectToEffectReferences(...). Because with PassRefPtr you give over the
ownership. This is maybe not what you want.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:48
> +    // They depend on the RenderObject argument of
RenderSVGResourceFilter::applyResource.

They is unclear, do you mean FilterEffects or the subregion of a filter effect?


> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:71
> +void
SVGFilterBuilder::appendEffectToEffectReferences(PassRefPtr<FilterEffect>
passEffectReference, RenderObject* object)

I wouldn't change RefPtr to PassRefPtr, as long as we don't know who should
take the ownership.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:57
> +    inline PassRefPtr<FilterEffect> getEffectByRenderer(RenderObject*
object) { return m_effectRenderer.get(object); }

getters don't begin with 'get' for WebCore, just effectByRenderer. You're not
calling this function anywhere, as far as I can see. Why did you add this here?
Can you change the name of getEffectReferences() as well?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:76
>      HashMap<RefPtr<FilterEffect>, FilterEffectSet> m_effectReferences;
> +    HashMap<RenderObject*, RefPtr<FilterEffect> > m_effectRenderer;

Do both hashmaps need to take RefPtr<FilterEffect>? Can we garantee somehow,
that if A has the element B has it as well? Would be better to save a pointer
in at least one map.