[webkit-reviews] review denied: [Bug 45812] Filter builder should be able to follow the filter object dependencies : [Attachment 67655] Draft patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 45812: Filter builder should be able to follow the filter object
dependencies
https://bugs.webkit.org/show_bug.cgi?id=45812

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67655&action=prettypatch

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:39
> +    RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes*
filterPrimitiveElement);
You should omit the argument name here, it's redundant.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:77
> +	   return effect;
I'd prefer a return 0 here.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:85
> +	   if (m_lastEffectReference1.get() != effect)
No need for the .get() here.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:91
> +    if (m_lastEffectReference1.get() == effect ||
m_lastEffectReference2.get() == effect)
Ditto. RefPtr defines operator*.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:95
> +	   if (m_lastEffectFurtherReferences[i].get() == effect)
Ditto.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:105
> +    ASSERT(m_effectReferences.find(effect) == m_effectReferences.end());
ASSERT(!m_effectReferences.contains(effect)): ?

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:108
> +    if (m_lastEffectReference1.get())
You can omit the .get()

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:110
> +    if (m_lastEffectReference2.get())
Ditto.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:112
> +    if (m_lastEffectFurtherReferences.size()) {
Use !isEmpty()

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:128
> +    ASSERT(!m_lastEffectFurtherReferences.size());
ASSERT(m_last..isEmpty())

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:46
>	   FilterEffect* getEffectById(const AtomicString& id) const;
The id parameter is redundant.

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:48
> +	   FilterEffect* getAndRecordEffectById(const AtomicString& id);
Ditto.

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:55
> +	       ASSERT(m_effectReferences.find(effect) !=
m_effectReferences.end());
ASSERT(m_effectReferences.contains(...)

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:67
> +	       ASSERT(m_effectReferences.find(effect) !=
m_effectReferences.end());
ASSERT(m_effectReferences.contains(...)

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:85
> +	   RefPtr<FilterEffect> m_lastEffectReference1;
It's not immediately clear to me, why those have to be refcounted at all.

I'll leave the actual review to Dirk, just wanted to share some notes.