[webkit-reviews] review denied: [Bug 5861] feConvolveMatrix filter is not implemented : [Attachment 60097] part 2: the implementation

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 5861: feConvolveMatrix filter is not implemented
https://bugs.webkit.org/show_bug.cgi?id=5861

Attachment 60097: part 2: the implementation
https://bugs.webkit.org/attachment.cgi?id=60097&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:158
 +     Where region C contains those pixels, which value
s/which value/whose values/

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:159
 +     can be calculated without crossing the edge of the rectange.
s/rectange/rectangle/

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:197
 +  static ALWAYS_INLINE unsigned char clampRGBAValue(float f)
I'd rename 'float f' to 'float rgba', to avoid abbreviations.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:199
 +	// Just a helper function
Doesn't help much to add this comment :-)

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:208
 +  template<bool preserveAlphaValues> ALWAYS_INLINE void
FEConvolveMatrix::fastSetInteriorPixels(PaintingData& paintingData, int
clipRight, int clipBottom)
I'd prefer a newline after the template<bool preserveAlphaValues>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:224
 +		totals[0] = 0.0f;
Juse use "= 0" here, it's the new style rule.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:231
 +		    totals[0] += m_kernelMatrix[kernelValue] *
static_cast<float>(paintingData.srcPixelArray->get(kernelPixel++));
Would it help to store m_kernelMatrix[kernelValue] in a local variable? Not
sure.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:244
 +		paintingData.dstPixelArray->set(pixel++,
clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
Is m_divisor guarenteed to be non null? If so please add an assertion on top of
this function.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:290
 +  template<bool preserveAlphaValues> void
FEConvolveMatrix::fastSetOuterPixels(PaintingData& paintingData, int x1, int
y1, int x2, int y2)
Please use a newline after the template<bool...>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:309
 +		totals[0] = 0.0f;
Use = 0, no 0.f postfix needed.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:333
 +		paintingData.dstPixelArray->set(pixel++,
clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
Same comment as above regarding m_divisor.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:355
 +	    fastSetInteriorPixels<true>(paintingData, clipRight, clipBottom);
Just wondering, is it possible to use m_preserveAlpha directly as template
parameter? I guess not :-)

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:93
 +	template<bool preserveAlphaValues> ALWAYS_INLINE void
fastSetInteriorPixels(PaintingData&, int clipRight, int clipBottom);
Use a newline after template<..>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:97
 +	template<bool preserveAlphaValues> void
fastSetOuterPixels(PaintingData&, int x1, int y1, int x2, int y2);
Ditto.

Great patch other than those small issues. Dirk might have additional comments?