[webkit-reviews] review denied: [Bug 19991] WebKit needs cross-platform filter system : [Attachment 30620] add new source input and filterBuilder

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 19991: WebKit needs cross-platform filter system
https://bugs.webkit.org/show_bug.cgi?id=19991

Attachment 30620: add new source input and filterBuilder
https://bugs.webkit.org/attachment.cgi?id=30620&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Dirk,

nice job, I have some comments though:

> Index: WebCore/platform/graphics/filters/SourceAlpha.cpp
...
> +PassRefPtr<SourceAlpha> SourceAlpha::create()
> +{
> +    return adoptRef(new SourceAlpha());
> +}
I'd prefer "new SourceAlpha" (without braces) here, not in the style guide but
I think it's general practice.

> Index: WebCore/platform/graphics/filters/SourceGraphic.cpp
...
> +PassRefPtr<SourceGraphic> SourceGraphic::create()
> +{
> +    return adoptRef(new SourceGraphic());
> +}
Ditto.

> Index: WebCore/svg/FilterBuilder.cpp
...
> + *  Copyright (C) Dirk Schulze <krit at webkit.org>
Year is missing?

> +FilterBuilder::FilterBuilder() {
> +    m_sourceAlpha = SourceAlpha::create();
> +    m_sourceGraphic = SourceGraphic::create();
> +}
Hm, I wonder wheter lazy creation of these objects would make sense? I think
this could save some time.

> +void FilterBuilder::add(const String& id, RefPtr<FilterEffect> effect)
> +{
> +	       m_lastEffect = effect.get();
> +
> +	       if(!id.isEmpty())
> +		   m_namedEffects.set(id.impl(), m_lastEffect); 
> +}
Wrong indention and if(!id) -> if (!id).

> +FilterEffect* FilterBuilder::getEffectById(const String& id) const
> +{   
> +    if(id.isEmpty() && m_lastEffect)
> +	   return m_lastEffect.get();
> +
> +    // If id isn't declared and lastEffect is still null then FilterBuilder
will return
> +    // a SourceGraphic object as per the Spec.
> +    if(id.isEmpty() && !m_lastEffect)
> +	   return m_sourceGraphic.get();
> +
> +    if (id == String("SourceGraphic"))
> +	   return m_sourceGraphic.get();
> +    if (id == String("SourceAlpha"))
> +	   return m_sourceAlpha.get();
> +    // FIXME: needs to be implemented
> +    if (id == String("BackgroundGraphic"))
> +	   return m_sourceGraphic.get();
> +    if (id == String("BackgroundAlpha"))
> +	   return m_sourceGraphic.get();
> +    if (id == String("FillPaint"))
> +	   return m_sourceGraphic.get();
> +    if (id == String("StrokePaint"))
> +	   return m_sourceGraphic.get();
> +
> +    return m_namedEffects.get(id.impl()).get();
> +}
Indention is wrong here, if( -> if ( has to be changed.
That function doesn't look particulary fast to me. I'd suggest to delegate the
work:

I'd suggest to add a new function to all FilterEffects: (example for
SourceGraphic)
<code>
static const AtomicString& SourceGraphic::filterName()
{
    DEFINE_STATIC_LOCAL(const AtomicString, s_filterName, ("SourceGraphic"));
    return s_filterName;
}
</code>

Then the getEffectById function could be written much nicer:

<code>
typedef HashMap<AtomicStringImpl*, FilterEffect*> FilterEffectMap;

FilterEffect* FilterBuilder::getEffectById(const AtomicString& id)
{
    DEFINE_STATIC_LOCAL(FilterEffectMap, s_filterEffects, ());

    if (s_filterEffects.isEmpty()) {
	s_filterEffects.set(SourceGraphic::filterName(),
m_sourceGraphic.get());
	s_filterEffects.set(SourceAlpha::filterName(), m_sourceAlpha.get());
	...
    }

    // If id isn't declared...
    if (id.isEmpty())
	return m_lastEffect ? m_lastEffect.get() : m_sourceGraphic.get();

    return s_filterEffects.get(id.impl());
}
</code>

If we'd go for lazy creation of the FilterEffect objects (m_sourceGraphic,
etc..) the HashMap could point to functions creating the FilterEffect, ie:
s_filterEffects.set(SourceAlpha::filterName(),
&FilterBuilder::createSourceAlpha())

FilterEffect* FilterBuilder::createSourceAlpha()
{
    if (m_sourceAlpha)
	return m_sourceAlpha.get();
    m_sourceAlpha = SourceAlpha::create()
    return m_sourceAlpha.get();
}
...

What do you think of this approach?

r- because of the ineffiency of the getEffectById function. It can easily be
fixed though.