[webkit-reviews] review granted: [Bug 19991] WebKit needs cross-platform filter system : [Attachment 30811] independent filter system

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted 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 30811: independent filter system
https://bugs.webkit.org/attachment.cgi?id=30811&action=review

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

> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================
> --- WebCore/platform/graphics/filters/Filter.h	(revision 0)
> +++ WebCore/platform/graphics/filters/Filter.h	(revision 0)
> @@ -0,0 +1,56 @@
> +/*
> + *  Copyright (C) 2009 Dirk Schulze <krit at webkit.org>
> + *
> + *	This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Library General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Library General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Library General Public
License
> + *  aint with this library; see the file COPYING.LIB.  If not, write to
> + *  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + *  Boston, MA 02110-1301, USA.
> + */
> +
> +#ifndef Filter_h
> +#define Filter_h
> +
> +#if ENABLE(FILTERS)
> +#include "FilterEffect.h"
> +#include "Image.h"
> +#include "PlatformString.h"
> +#include "StringHash.h"
String includes don't seem to be needed here at all. Same for "FilterEffect.h",
can use a class forward for that (you're even doing that, so the include is
unnecessary).

> +
> +#include <wtf/PassOwnPtr.h>
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/RefPtr.h>
I don't see any RefPtr/PassRefPtr usage here?

> +
> +namespace WebCore {
> +
> +    class AtomicString;
AtomicString is superflous.

> +    class FilterEffect;
> +
> +    class Filter : public RefCounted<Filter> {
> +    public:
> +
> +	   void setSourceImage(PassOwnPtr<Image> image) { m_image = image; }
> +	   Image* sourceImage() { return m_image.get(); }
> +
> +	   virtual void filterEffectSubRegion(FilterEffect*) = 0;
Isn't this function supposed to return anything?
If not, it should be renamed to sth. like "calculateEffectSubRegion".

> +
> +    private:
> +
> +	   OwnPtr<Image> m_image;
You can omit that newline here, after "private:".


> Index: WebCore/platform/graphics/filters/FilterEffect.h
> ===================================================================
> --- WebCore/platform/graphics/filters/FilterEffect.h	(revision 44295)
> +++ WebCore/platform/graphics/filters/FilterEffect.h	(working copy)
> @@ -22,9 +22,9 @@
>  #define FilterEffect_h
>  
>  #if ENABLE(FILTERS)
> +#include "Filter.h"
>  #include "FloatRect.h"
>  #include "ImageBuffer.h"
> -#include "SVGResourceFilter.h"
>  #include "TextStream.h"
>  
>  #include <wtf/PassRefPtr.h>
> @@ -33,6 +33,8 @@
>  
>  namespace WebCore {
>  
> +    class Filter;
> +
>      class FilterEffect : public RefCounted<FilterEffect> {
>      public:
>	   virtual ~FilterEffect();
> @@ -56,7 +58,7 @@ namespace WebCore {
>	   ImageBuffer* resultImage() { return m_effectBuffer.get(); }
>	   void setEffectBuffer(ImageBuffer* effectBuffer) {
m_effectBuffer.set(effectBuffer); }
>  
> -	   virtual void apply(SVGResourceFilter*) = 0;
> +	   virtual void apply(Filter*) = 0;
>	   virtual void dump() = 0;
>  
>	   virtual TextStream& externalRepresentation(TextStream&) const;
Either use a class forward for "Filter" or an include, not both.


> Index: WebCore/svg/graphics/filters/SVGFilter.h
....

> +	   void filterEffectSubRegion(FilterEffect*);
Same comment as above, needs a clarification.

> +
> +    private:
> +	   SVGFilter(const FloatRect& itemBox, const FloatRect& filterRect,
bool itemBBoxMode, bool filterBBoxMode);
> +
> +	   FloatRect m_itemBox;
> +	   FloatRect m_filterRect;
> +	   bool m_effectBBoxMode;
> +	   bool m_filterBBoxMode;
> +    };
Okay the constructor doesn't look that nice. We can leave it as is for now, but
it might be nicer to use an enum with flags
and pass that as constructor argument instead of two bools (thinkin of
"EffectBoundingBox | FilterBoundingBox"..).

Nontheless, r=me. You can fix above things before landing.