[webkit-reviews] review denied: [Bug 6021] WebKit+SVG does not support filterRes attribute : [Attachment 43448] Implementation of filterRes

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 6021: WebKit+SVG does not support filterRes attribute
https://bugs.webkit.org/show_bug.cgi?id=6021

Attachment 43448: Implementation of filterRes
https://bugs.webkit.org/attachment.cgi?id=43448&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Jeeezz, what a large patch, thank god I'm on a nightshift with nothing else to
do ;-)

Several comments:
- You need a much more detailed ChangeLog, as this is a large patch, ideally
per file, this time.

> Index: WebCore/platform/graphics/filters/FEGaussianBlur.cpp
...
> +    sdx = std::max(sdx, static_cast<unsigned>(1));
> +    sdy = std::max(sdy, static_cast<unsigned>(1));

Why not just std::max(sdx/y, 1); ?

> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================
> --- WebCore/platform/graphics/filters/Filter.h	(revision 51070)
> +++ WebCore/platform/graphics/filters/Filter.h	(working copy)
> @@ -22,6 +22,7 @@
>  
>  #if ENABLE(FILTERS)
>  #include "FloatRect.h"
> +#include "FloatSize.h"
>  #include "ImageBuffer.h"
>  #include "StringHash.h"
>  
> @@ -40,15 +41,21 @@ namespace WebCore {
>	   void setSourceImage(PassOwnPtr<ImageBuffer> sourceImage) {
m_sourceImage = sourceImage; }
>	   ImageBuffer* sourceImage() { return m_sourceImage.get(); }
>  
> +	   FloatSize filterRes() const { return m_filterRes; }
> +	   void setFilterRes(const FloatSize& filterRes) { m_filterRes =
filterRes; }

Hrm, I'd prefer to completly avoid abbrevations, aka. change it to
"setFilterResolution" here.
Filter* has no direct connection to SVG, and it's not in the "// SVG specific"
section below, so why not rename it?
We don't need to reuse SVG naming conventions here.

  
> Index: WebCore/svg/SVGFilterElement.cpp
...
> +    else if (attr->name() == SVGNames::filterResAttr) {
> +	   float x, y;
> +	   if (parseNumberOptionalNumber(value, x, y)) {
> +	       setFilterResXBaseValue(x);
> +	       setFilterResYBaseValue(y);
> +	   }

An else case reporting errors through SVGDocumentExtensions seems to be missing
here.

> +    if (this->hasAttribute(SVGNames::filterResAttr)) {
s/this->//

> +	   m_filter->setHasFilterRes(true);
> +	   m_filter->setFilterResSize(FloatSize(filterResX(), filterResY()));

Same thoughts as in Filter.h:
s/setHasFilterRes/setHasFilterResolution/
s/setFilterResSize/setFilterResolution/ (Size prefix is superflous IMHO)

> Index: WebCore/svg/graphics/SVGResourceFilter.cpp
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.cpp	(revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.cpp	(working copy)
> @@ -35,6 +35,10 @@
>  #include "SVGRenderTreeAsText.h"
>  #include "SVGFilterPrimitiveStandardAttributes.h"
>  
> +#define MAX_FILTER_SIZE 5000.f

Evil defines, please use a static constant here. I am not sure about our naming
convention, it may have a g, or k prefix, please check for common usage of
static constants in other files.

> +bool SVGResourceFilter::matchesMaxImageSize(const FloatSize& size) {

{ neeeds to go on a new line

> +    bool matchesFilterSize = true;
> +    if (size.width() > MAX_FILTER_SIZE) {
> +	   m_scaleX *= MAX_FILTER_SIZE / size.width();
> +	   matchesFilterSize = false;
> +    }
> +    if (size.height() > MAX_FILTER_SIZE) {
> +	   m_scaleY *= MAX_FILTER_SIZE / size.height();
> +	   matchesFilterSize = false;
> +    }
> +
> +    return matchesFilterSize;
> +}

matchesMaxImageSize is not a good name. How about "fitsInMaximumImageSize" or
something similar.
Matches sounds too much like "is equal".


>  void SVGResourceFilter::prepareFilter(GraphicsContext*& context, const
RenderObject* object)
...
> +    // scale to big sourceImage size to MAX_FILTER_SIZE
> +    tempSourceRect.scale(m_scaleX, m_scaleY);
> +    matchesMaxImageSize(tempSourceRect.size());

Hrm, you're ignoring the return value of matchesMaxImageSize here??


> +
>      // prepare Filters
>      m_filter = SVGFilter::create(targetRect, m_filterBBox,
m_effectBBoxMode);
> +    m_filter->setFilterRes(FloatSize(m_scaleX, m_scaleY));
>  
>      FilterEffect* lastEffect = m_filterBuilder->lastEffect();
> -    if (lastEffect)
> +    if (lastEffect) {
>	   lastEffect->calculateEffectRect(m_filter.get());
> +	   // at least one FilterEffect has a to big image size,
s/to/too/

>  void SVGResourceFilter::applyFilter(GraphicsContext*& context, const
RenderObject*)
>  {
> +    if (!m_scaleX || !m_scaleY || !m_filterBBox.width() ||
!m_filterBBox.height())
> +	   return;

The same check is used as above, might make sense to refactor in a simple
"static inline bool shouldProcessFilter()" helper function?

> Index: WebCore/svg/graphics/SVGResourceFilter.h
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.h	(revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.h	(working copy)
> @@ -53,6 +53,9 @@ public:
>      
>      virtual SVGResourceType resourceType() const { return
FilterResourceType; }
>  
> +    void setFilterResSize(const FloatSize& filterResSize) { m_filterResSize
= filterResSize; }
> +    void setHasFilterRes(bool filterRes) { m_filterRes = filterRes; }
Same rename suggestions apply as above.

> +    bool matchesMaxImageSize(const FloatSize&);
Ditto.

> +    FloatSize m_filterResSize;
I'd suggest just "m_filterResolution" here.

> Index: WebCore/svg/graphics/filters/SVGFilter.h
...
> -	   bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
> +	   virtual bool effectBoundingBoxMode() const { return
m_effectBBoxMode; }
>  
> -	   FloatRect filterRegion() const { return m_filterRect; }
> -	   FloatRect sourceImageRect() const { return m_itemBox; }
> -	   void calculateEffectSubRegion(FilterEffect*) const;
> +	   virtual FloatRect filterRegion() const { return m_filterRect; }
> +	   virtual FloatRect sourceImageRect() const { return m_itemBox; }
> +
> +	   virtual FloatSize maxImageSize() const { return m_maxImageSize; }
> +	   virtual void calculateEffectSubRegion(FilterEffect*);

I guess these functions have already been marked virtual in the base class, and
this is just for clarification?

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 51126)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> +2009-11-18  Dirk Schulze  <krit at webkit.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   The first two example have a filter with a size of 20000x20000. We
limit
> +	   the quality to be able to render this filter.
> +	   On the third example the filter qulitiy is limited by different
values
> +	   to test the bahavior of effects with kernels or other values that
need
> +	   to be scaled too. It also tests the correctness of the subRegion
calculation
> +	   for filters with more than one effect.
Better name the tests, instead of relying on the reviewers ability to count ;-)


Just realized the patch is not that big, it's mostly the pngs, that create a
frightning big patch size.
Please fix these issues, and upload a new patch. I think the general concept
looks sane.

Wouldn't hurt if Simon Fraser / Eric Seidel / Oliver Hunt may have another
look. I'll cc them soon.

r- because of the mentioned issues above.