[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





--- Comment #9 from Zoltan Herczeg <zherczeg at webkit.org>  2010-09-22 07:06:13 PST ---
> > 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?

If we would use a 'set', we would also create a new 'set' for each effect:
+    m_effectReferences.add(effect, FilterEffectVector());
would be
+    m_effectReferences.add(effect, set());

Using a vector here is a speed optimization, since iterating through a vector (needed by the 'invalidation' process) is much cheaper than iterating through a set. Moreover, we only append items during initialization, but invalidation will use the iteration many times.

> > 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.

Nobody at the moment, but it will be needed in the future, I am sure.

> > 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.

It is called from the constructor: 

SVGFilterBuilder::SVGFilterBuilder()
{
    m_builtinEffects.add(SourceGraphic::effectName(), SourceGraphic::create());
    m_builtinEffects.add(SourceAlpha::effectName(), SourceAlpha::create());
    addBuiltinEffects();
}

What do you mean by 'initialization'?

I am agree with the other things.

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