[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 #10 from Dirk Schulze <krit at webkit.org>  2010-09-23 00:31:16 PST ---
(In reply to comment #9)
> > > 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());
Your're doing it anyway two lines later. And it doesn't matter if you check it with you temporary set or with the set of EffectReferences. Or do I get something wrong? Later you want to update the ReferenceList and it makes no sense to write your own iterator for the vector, if you could just call contains(..)

> 
> 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.
Please add it at this time when we need it.

> 
> > > 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'?
Exactly that :-) But we call clearFilterBuilder in the destructor. And clearFilterBuilder calls your function that adds the pseudo effects. That needs to change somehow. Not sure why we call clear on RenderSVGResourceFilter right after creating the Builder. Maybe that should be removed as well.

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