[Webkit-unassigned] [Bug 45812] Filter builder should be able to follow the filter object dependencies

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67655|                            |review-
               Flag|                            |




--- Comment #2 from Nikolas Zimmermann <zimmermann at kde.org>  2010-09-15 03:38:51 PST ---
(From update of attachment 67655)
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.

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