[webkit-reviews] review denied: [Bug 42273] Convolution computation causes bad alpha channel values : [Attachment 61543] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Alex Nicolaou
<anicolao at chromium.org>'s request for review:
Bug 42273: Convolution computation causes bad alpha channel values
https://bugs.webkit.org/show_bug.cgi?id=42273

Attachment 61543: Patch
https://bugs.webkit.org/attachment.cgi?id=61543&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
LayoutTests/ChangeLog:14
 +	    * svg/custom/sigfpe.svg: Added.
This doesn't seem like a very useful name for a layout test.

LayoutTests/svg/custom/sigfpe.svg:5
 +  <rect x="10" y="10" width="20" height="20" fill="green"/>
historicaly 100x100 green rect has meant success in graphical tests.  I'm not
sure this test even needs pixel results though.  <text> which said PASS would
be sufficient.	Then you could mark it as layoutTestController.dumpAsText().

WebCore/platform/graphics/skia/SkiaUtils.cpp:89
 +	    SkASSERT(false); // invalid colour channels; r,g,b should be 0 if
a=0
Please use full english sentences, beginning with a capital and ending in a
period.

WebCore/platform/graphics/skia/SkiaUtils.cpp:90
 +	    // protect against sigfpe for production
divide by zero might be a more common name for SIGFPE.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:251
 +		    paintingData.dstPixelArray->set(pixel++,
std::min(clampRGBAValue(totals[0] / m_divisor + paintingData.bias), alpha));
Bah.  This should be a function of some form.  Copy/pasting 4 lines in a row is
silly.