Created attachment 59024[details]
Test example
I've attached an html file where it is easy to check the problem that it causes, it clips the shadows. It is also a problem for the bug where we are trying to do tiling in order to create the shadow because we need the size of the kernel in order to get the minimum size of a rectangle (bug 39582)
39582
After talking to krit an hixie I have also uploaded a bug to the HTML5 standard to avoid misinterpretations of the sentence describing the shadowBlur: http://www.w3.org/Bugs/Public/show_bug.cgi?id=9942

Created attachment 59026[details]
Initial patch proposal
This is an initial example of what we can do, the other option would be to create the filters in before calling the GraphicContext functions. Open to comments :).

Attachment 60279[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:101: Should have a space between // and comment [whitespace/comments] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:104: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:113: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:121: One line control clauses should not use braces. [whitespace/braces] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:126: GAUSSIAN_KERNEL_FACTOR is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:182: Missing spaces around / [whitespace/operators] [3]
WebCore/platform/graphics/filters/FEGaussianBlur.cpp:183: Missing spaces around / [whitespace/operators] [3]
Total errors found: 8 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 60298[details]
Proposed patch
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.

(In reply to comment #5)
>
> [...]
>
> I'm quite sure, that this need updates of pixel tests. Can you run layout tests with a tolerance of zero please?
Yep, the visual result is different, I was wondering if other platforms with pixel tests for this filter could have problems because I could not test it in webkitgtk+. I'll review what we need.
> 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.
I understand, we can change also this condition that controls the standard deviation parameter:
if (m_x == 0 || m_y == 0)
return;
with
if (m_x == 0 && m_y == 0)
return;
And add ifs to the calculations to avoid calculating sdx or sdy and avoid the blox blur in that direction.
Probably the name of the parameters/variables are still confusing, I could change m_x and m_y with mStdx and mStdy, and sdx/sdy with kernelSizex/kernelSizey.
> 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.
Yep, actually it was my my initial intention to do it separately, but finally checking the visual result of reviewing the kernel_size I've checked the position of the shadow was not correct without all the modifications, and I was not sure if it was due to the change I did, my fault, I'll check it and try to separate both patches adding the kernelPosition. In case we have pixel tests we will have to do two modifications also.
> Great patch! Please fix the style problems and think about the last comment about kernelPosition. r- for now, also for breaking Mac.
I'll do, thanks very much for the review Dirk :).

(In reply to comment #6)
> (In reply to comment #5)
> >
> > [...]
> >
> > I'm quite sure, that this need updates of pixel tests. Can you run layout tests with a tolerance of zero please?
>
> Yep, the visual result is different, I was wondering if other platforms with pixel tests for this filter could have problems because I could not test it in webkitgtk+. I'll review what we need.
>
> > 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.
>
> I understand, we can change also this condition that controls the standard deviation parameter:
>
> if (m_x == 0 || m_y == 0)
> return;
>
> with
>
> if (m_x == 0 && m_y == 0)
> return;
The result of the filter, if stdDeviation is 0 for all sizes, is as if no filter was applied at all. that means input = output. With if (m_x == 0 && m_y == 0) the output is transparent black, independent of the input. The spec changed lately and noone removed this check up to now.
>
> And add ifs to the calculations to avoid calculating sdx or sdy and avoid the blox blur in that direction.
>
> Probably the name of the parameters/variables are still confusing, I could change m_x and m_y with mStdx and mStdy, and sdx/sdy with kernelSizex/kernelSizey.
For private variables of a class we use m_... as prefix. But of course you can rename it to something like m_stdX and m_stdY.
> I'll do, thanks very much for the review Dirk :).
Thank you for working on this problem :-)

Hi,
I was wondering why are we limiting the radius to 1000? It is true that CG is doing this. However, skia in *http://code.google.com/p/skia/source/browse/trunk/src/effects/SkBlurMaskFilter.cpp* (MAX_RADIUS) is limiting it to 128? For performance reasons, wouldn't it be better to follow skia model here and limit the radius values even more? It seems to me that values greater than 128 are both computational intensive and unlikely to be used in practice.
Regards,
Mihnea Ovidenie

(In reply to comment #8)
> Hi,
>
> I was wondering why are we limiting the radius to 1000? It is true that CG is doing this. However, skia in *http://code.google.com/p/skia/source/browse/trunk/src/effects/SkBlurMaskFilter.cpp* (MAX_RADIUS) is limiting it to 128? For performance reasons, wouldn't it be better to follow skia model here and limit the radius values even more? It seems to me that values greater than 128 are both computational intensive and unlikely to be used in practice.
>
I agree, I think we can use deviation instead of radius and do it inside the FEGaussianBlur object that way, we can use 50 for instance which is a radius of 94.5 aprox. I think mozilla is using 25, at leas in the canvas.

Created attachment 61538[details]
Proposed patch
After trying to add all the comments I've disagreed with myself and agreed with Mihnea about the limit :), adding it to the deviation was not as natural as using the radius.
This patch depends on the patch in the bug 41605. I'm not adding it for review until we land the other patch.

(In reply to comment #18)
¡> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:203
> + void FEGaussianBlur::calculateStdDeviation(const FloatPoint& radius, const FloatSize& resolution, float& sdx, float& sdy)
> Why did you add this? IIRC we just need the code for CSS shadows. We don't have different radii for width or height. The resolution is always FloatSize(1,1). So why not just add the following line once directly to the Shadow code in GC?
>
The main point to add the function is keep all the gaussian calculation in the same file, it allow us to reuse calculations like the gGaussianKernelFactor for this and to get the kernel size in the apply method.
The reason to leave all the width, height even though they are the same for css is to keep the API like a general way to get the standard deviation from the radius. I could make the function more specific for this case if that fits better.

Hm, FEGaussianBlur::calculateStdDeviation is a void but it's only task is to calculate a float, can't you just give back a float?
float sd = FEGaussianBlur::calculateStdDeviation(...)
Also please use: float FEGaussianBlur::calculateStdDeviation(float radius) const