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

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 45812: Filter builder should be able to follow the filter object
dependencies
https://bugs.webkit.org/show_bug.cgi?id=45812

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

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

Please add a ChangeLog with detailed descriptions next time. The patch looks
great and I'm looking forward to see the next patch.

> WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:89
> +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect>
effect)
> +{
> +    // The effect must be a newly created filter effect.
> +    ASSERT(!m_effectReferences.contains(effect));
> +    m_effectReferences.add(effect, FilterEffectVector());
> +
> +    FilterEffectVector& inputEffects = effect->inputEffects();
> +    HashSet<RefPtr<FilterEffect> > seenEffects;
> +    int size = inputEffects.size();
> +
> +    for (int i = 0; i < size; ++i) {
> +	   // Since targetEffect is a newly created object, it cannot already
be added to the list.
> +	   ASSERT(m_effectReferences.contains(inputEffects[i]));
> +	   if (seenEffects.contains(inputEffects[i]))
> +	     continue;
> +	   m_effectReferences.find(inputEffects[i])->second.append(effect);
> +	   seenEffects.add(inputEffects[i]);
> +    }
> +}

I'm not sure why you want to have a FilterEffectVector instead of a Set? At the
moment you create a set for every call of  appendEffectToEffectReferences.
Wouldn't it be more useful to put a set into m_effectReferences:
HashMap<RefPtr<FilterEffect>, Set<RefPtr<FilterEffect> > > ? So you just need
check the set, if the effect is already in there: contains(effect), and it if
not. Niko already asked it, do we need the RefPtr's? Can't we just store the
pointer?

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:51
> +	   inline FilterEffectVector& getReferences(FilterEffect* effect)
> +	   {
> +	       // Only allowed for effects belongs to this builder.
> +	       ASSERT(m_effectReferences.contains(effect));
> +	       return m_effectReferences.find(effect)->second;
> +	   }

Who is using that? Also this one liner don't need to be in an extra function.

> WebCore/svg/graphics/filters/SVGFilterBuilder.h:63
> +	   inline void addBuiltinEffects()
> +	   {
> +	       HashMap<AtomicString, RefPtr<FilterEffect> >::iterator end =
m_builtinEffects.end();
> +	       for (HashMap<AtomicString, RefPtr<FilterEffect> >::iterator
iterator = m_builtinEffects.begin(); iterator != end; ++iterator)
> +		    m_effectReferences.add(iterator->second,
FilterEffectVector());
> +	   }

You should call this during initialization. Not sure if we get a leak if we
leaf it in on clearEffects and call the DTor of FilterBuilder.