[webkit-reviews] review denied: [Bug 40793] [GTK] The size of the shadow image uses the standard deviation size instead of the blur radius : [Attachment 60298] 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 40793: [GTK] The size of the shadow image uses the standard deviation size
instead of the blur radius
https://bugs.webkit.org/show_bug.cgi?id=40793

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
WebCore/platform/graphics/cairo/FontCairo.cpp:97
 +	    float radius = 0.f;
Please use jut 0 without the .f at the end (according to the change in the
style guide)

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:180
 +	float radius = 0.f;
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:580
 +	float radius = 0.f;
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:871
 +	radius = std::min(1000.f, radius);
dito

WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:873
 +	FloatSize resolution = FloatSize(1.0f, 1.0f);
...(1, 1) Also, the resolution doesn't change here, add it to the funtion call
directly

WebCore/platform/graphics/cairo/ImageCairo.cpp:143
 +	    float radius = 0.f;
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:99
 +  static void kernelPosition(int boxBlur, unsigned std, int& dLeft, int&
dRight)
Either you use inline, or you add the function to the header. I bet that is the
reason why Mac-build fails.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:125
 +  static const float gaussianKernelFactor = (3.f * sqrt(2 * M_PI) / 4.f);
Please add this line right after the includes. Name it gGaussianKernelFactor to
make sure that this is a constant. No .f. I think M_PI is wrong here (problems
on Mac builds). please use piDouble again.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
 +	unsigned sdx = static_cast<unsigned>(floor(std::max(m_x, 2.f) *
filter->filterResolution().width() * gaussianKernelFactor + 0.5f));
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
 +	unsigned sdy = static_cast<unsigned>(floor(std::max(m_y, 2.f) *
filter->filterResolution().height() * gaussianKernelFactor + 0.5f));
dito

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:181
 +	sdx = std::max((radius.x() * 2 / 3 - 0.5f) / (resolution.width() *
gaussianKernelFactor), 0.f);
Please use: using namespace std; Saw this multiple times before, please fix it
there as well.
no .f

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:182
 +	sdy = std::max((radius.y() * 2 / 3 - 0.5f) / (resolution.height() *
gaussianKernelFactor), 0.f);
dito

I'm quite sure, that this need updates of pixel tests. Can you run layout tests
with a tolerance of zero please? I'm also not sure about kernelPosition. The
spec for feGaussianBlur changed recently and a blurring radius of 0 is allowed
now. If a stdDeviation for any direction is zero, the image is just not blurred
for this direction. At the moment we break and don't draw the blurring at all.
Now that you try to fix the SVG filter as well, and I'm realy glad about this
:-), can you take about this fact as well? It might also be possible to move
the feGaussianBlur fixes to another patch,if you think this is not the right
place.

Great patch! Please fix the style problems and think about the last comment
about kernelPosition. r- for now, also for breaking Mac.