[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


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68357|review?                     |review-
               Flag|                            |




--- Comment #8 from Dirk Schulze <krit at webkit.org>  2010-09-22 05:31:27 PST ---
(From update of attachment 68357)
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.

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