[webkit-reviews] review denied: [Bug 41605] Fixes to the gaussian blur algorithm : [Attachment 60652] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 41605: Fixes to the gaussian blur algorithm
https://bugs.webkit.org/show_bug.cgi?id=41605

Attachment 60652: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=60652&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:38
 +  static const float gGaussianKernelFactor = (3 * sqrt(2 * piDouble) / 4);
Again, was my mistake, see comment above. Can you delete #include <math.h> as
well please? Shouldn't be needed.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:150
 +	unsigned kernelSizeX = static_cast<unsigned>(floor(max(m_stdX,
static_cast<float>(2.0)) * filter->filterResolution().width() *
gGaussianKernelFactor + 0.5));
Isn't it enough to write 2.f instead of static_cast<float>(2.0)? Please use
0.5f. Only if you have a whole-number, you can omit .0 or .f (not sure about
the denominator on a division).

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:151
 +	unsigned kernelSizeY = static_cast<unsigned>(floor(max(m_stdY,
static_cast<float>(2.0)) * filter->filterResolution().height() *
gGaussianKernelFactor + 0.5));
dito

To the LayoutTests.. You changes behavior, so you need some more layoutTests to
cover the changes. I suggest a test, with a stdDerivation of "0", "0 3" and "3
0". According to the new spec, we should see results in all three cases (not
the case at the moment).
You changed the result of one of the tests, so that other platforms don't need
new test results? That's not the regular case to do so :-) If you don't have a
Mac, or no access to a Mac, I can create the results for you.
The patch misses a ChangeLog entry for LayoutTests.

Great patch! Hope to see an update soon. r- for the few style issues and the
missing LayoutTests/Changelog-entry.