Bug 5861 - feConvolveMatrix filter is not implemented
: feConvolveMatrix filter is not implemented
Status: RESOLVED FIXED
: WebKit
SVG
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
: 41011
: 5857 26389
  Show dependency treegraph
 
Reported: 2005-11-28 17:19 PST by
Modified: 2010-07-01 03:14 PST (History)


Attachments
SVG feConvolveMatrix implementation (41.64 KB, patch)
2009-11-08 11:25 PST, Dirk Schulze
oliver: review-
Review Patch | Details | Formatted Diff | Diff
SVG feConvolveMatrix tests (65.51 KB, patch)
2009-11-08 11:28 PST, Dirk Schulze
eric: review-
Review Patch | Details | Formatted Diff | Diff
draft patch (continue to work on Dirk's implementation) (47.53 KB, patch)
2010-06-10 01:14 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
base patch (contains the build and other things, many things come from Dirk's patch) (47.48 KB, patch)
2010-06-14 04:42 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
next try (49.75 KB, patch)
2010-06-14 23:54 PST, Zoltan Herczeg
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
base patch (52.29 KB, patch)
2010-06-23 02:13 PST, Zoltan Herczeg
krit: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch - landed in r62092 (56.25 KB, patch)
2010-06-28 03:39 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
part 2: the implementation (47.53 KB, patch)
2010-06-30 01:31 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
part 2: the implementation (114.43 KB, patch)
2010-06-30 01:33 PST, Zoltan Herczeg
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
updated patch (114.59 KB, patch)
2010-07-01 01:01 PST, Zoltan Herczeg
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-11-28 17:19:34 PST

    
------- Comment #1 From 2009-11-08 11:25:08 PST -------
Created an attachment (id=42717) [details]
SVG feConvolveMatrix implementation

SVG feConvolveMatrix implementation
------- Comment #2 From 2009-11-08 11:28:01 PST -------
Created an attachment (id=42718) [details]
SVG feConvolveMatrix tests

Some further tests for feConvolveMatrix.
------- Comment #3 From 2009-11-09 13:37:58 PST -------
(From update of attachment 42717 [details])
I don't like the _foo names, i think it would be better if you did

foo = m_foo;

if (this->hasAttribute(SVGNames::divisorAttr) && _divisor == 0.f)

What happens if divisor is NaN ?

In the inner loop of FEConvolveMatrix::apply seems really horrifyingly expensive -- at the very lesat i think the edge case checks should be hoisted out of the main loop and done separately.
------- Comment #4 From 2009-11-25 22:08:11 PST -------
(From update of attachment 42718 [details])
Test looks OK.  Why are the results platform specific?
------- Comment #5 From 2010-06-10 01:14:43 PST -------
Created an attachment (id=58342) [details]
draft patch (continue to work on Dirk's implementation)
------- Comment #6 From 2010-06-14 04:42:22 PST -------
Created an attachment (id=58634) [details]
base patch (contains the build and other things, many things come from Dirk's patch)
------- Comment #7 From 2010-06-14 04:45:57 PST -------
Attachment 58634 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGFEConvolveMatrixElement.cpp:112:  _kernelMatrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:120:  _orderX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:121:  _orderY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:129:  _targetX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:130:  _targetY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:140:  _divisor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:141:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:146:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:153:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:35:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 11 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #8 From 2010-06-14 05:30:59 PST -------
Attachment 58634 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3282105
------- Comment #9 From 2010-06-14 23:54:33 PST -------
Created an attachment (id=58758) [details]
next try
------- Comment #10 From 2010-06-14 23:56:09 PST -------
Attachment 58758 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGFEConvolveMatrixElement.cpp:112:  _kernelMatrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:120:  _orderX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:121:  _orderY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:129:  _targetX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:130:  _targetY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:140:  _divisor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:141:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:146:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:153:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:35:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 11 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #11 From 2010-06-15 00:52:12 PST -------
Attachment 58758 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3279154
------- Comment #12 From 2010-06-22 13:45:21 PST -------
The cr-ews incorrectly reprots an error. Anyone up for a review?
------- Comment #13 From 2010-06-23 00:17:15 PST -------
(In reply to comment #12)
> The cr-ews incorrectly reprots an error. Anyone up for a review?
No, there is a problem with V8SVGFEConcolveMatrix, can you look into that please? Also there a several style fixes that can easily be fixed.

I'll give your patch a review soon as well.
------- Comment #14 From 2010-06-23 00:46:13 PST -------
(From update of attachment 58758 [details])
WebCore/GNUmakefile.am:3411
 +      WebCore/svg/SVGFEConvolveMatrixElement.cpp \
Leading space.

WebCore/GNUmakefile.am:3412
 +      WebCore/svg/SVGFEConvolveMatrixElement.h \
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:59
 +          if (parseNumberOptionalNumber(value, x, y)) {
What happens if parsing failed, you might want to reset order x/y then? Think of scripting feConvolveMatrix. We have other code like this, that handles the error case.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:71
 +          kernelMatrixBaseValue()->parse(value);
What about the error case?

WebCore/svg/SVGFEConvolveMatrixElement.cpp:73
 +          setDivisorBaseValue(value.toFloat());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:75
 +          setBiasBaseValue(value.toFloat());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:77
 +          setTargetXBaseValue(value.toUIntStrict());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:79
 +          setTargetYBaseValue(value.toUIntStrict());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:82
 +          if (parseNumberOptionalNumber(value, x, y)) {
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:112
 +      Vector<float> _kernelMatrix;
Style issue, as the bot reported.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:117
 +      for (int i = 0; i < nr; i++)
for (int i = 0; i < numberOfItems; ++i)

Please don't use abbrevations.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:120
 +      int _orderX = orderX();
Style issue, need to rename, to avoid leading "_".

WebCore/svg/SVGFEConvolveMatrixElement.cpp:122
 +      if (!this->hasAttribute(SVGNames::orderAttr)) {
No need to use "this->" prefix.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:126
 +      if (_orderX * _orderY != nr)
This needs a comment.



WebCore/svg/SVGFEConvolveMatrixElement.cpp:129
 +      int _targetX = targetX();
Style issue as well. Same for _targetY.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:131
 +      if (this->hasAttribute(SVGNames::targetXAttr) && (_targetX < 0 || _targetX >= _orderX))
"this->" not needed. Needs a comment explaining the range of this value.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:133
 +      if (!this->hasAttribute(SVGNames::targetXAttr))
"this->" not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:134
 +          _targetX = static_cast<int>(floor(_orderX / 2));
Needs a comment, why this is the default.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:135
 +      if (this->hasAttribute(SVGNames::targetYAttr) && (_targetY < 0 || _targetY >= _orderY))
"this->" not needed. Needs a comment explaining the range of this value.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:137
 +      if (!this->hasAttribute(SVGNames::targetYAttr))
"this->" not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:139
 +  
Needs a comment, why this is the default.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:141
 +      if (this->hasAttribute(SVGNames::divisorAttr) && _divisor == 0.f)
"this->" not needed. Shouldn't this check include negative values, then you could just check if (hasAttribute(SVGnames::divisorAttr) && !divisor)

WebCore/svg/SVGFEConvolveMatrixElement.cpp:143
 +      if (!this->hasAttribute(SVGNames::divisorAttr)) {
this-> not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:146
 +          if (_divisor == 0.f)
Maybe just if (!divisor) divisor = 1;
No .f postfixes needed, per style rules.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:147
 +              _divisor = 1.f;
No .f needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:150
 +      return FEConvolveMatrix::create(input1, IntSize(_orderX, _orderY), _divisor, bias(), IntPoint(_targetX, _targetY), static_cast<EdgeModeType>(edgeMode()), FloatPoint(kernelUnitLengthX(), kernelUnitLengthX()), preserveAlpha(), _kernelMatrix);
You could wrap this in multiple lines with "commas" at the end, lining up at the create( opening brace.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34
 +  using std::min;
Please use "using namespace std;".

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:42
 +      const float& divisor, const float& bias, const IntPoint& targetOffset, EdgeModeType edgeMode,
No const references for POD types needed., use just "float".

Please fix all style issues (you can easily do that, after running check-webkit-style on a patchset before submitting). Also the V8 problem is valid. No idea how to fix it, grep how the existing bindings work :-)
Other than style issues a great first start!
------- Comment #15 From 2010-06-23 02:13:22 PST -------
Created an attachment (id=59494) [details]
base patch

The ChangeLog will be something like this:

The patch was started by Dirk Schulze, and continued later by me. The patch was split into two parts: this first part contains a new ConvolveMatrixElement class, and the necessary build modifications. It does not, however, a paint implementation for convolve matrix filter.
------- Comment #16 From 2010-06-23 02:17:26 PST -------
My notes:

Chromium ews incremental build feature needs fixed to detect new .idl files. I have opened a new bug report, and added a dependency for this patch (41011)

Most of the stlye bugs comes from the old patch, since the style was different at that time, and they are easy fixes.

> WebCore/svg/SVGFEConvolveMatrixElement.cpp:59
>  +          if (parseNumberOptionalNumber(value, x, y)) {
> What happens if parsing failed, you might want to reset order x/y then? Think of scripting feConvolveMatrix. We have other code like this, that handles the error case.

Other SVG elements do their parsing this way, so this is probably ok.
------- Comment #17 From 2010-06-23 02:35:04 PST -------
Attachment 59494 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3318681
------- Comment #18 From 2010-06-23 04:27:06 PST -------
(From update of attachment 59494 [details])
Don't open new br's. The builds must clearly work with this patch. Search in the history of trac.webkit.org for vkern. The initial commit and some following commits will give you a hint what you need to change to get chromium building.
------- Comment #19 From 2010-06-28 03:39:27 PST -------
Created an attachment (id=59890) [details]
Updated patch
------- Comment #20 From 2010-06-28 04:32:48 PST -------
> Don't open new br's. The builds must clearly work with this patch. 

Hey Dirk, this was really a bug with the Chromium-ews, they fixed it and now the build works.
------- Comment #21 From 2010-06-28 09:16:10 PST -------
> Hey Dirk, this was really a bug with the Chromium-ews, they fixed it and now the build works.

Yay.  Glad the fix worked.  :)
------- Comment #22 From 2010-06-28 23:17:16 PST -------
(From update of attachment 59890 [details])
WebCore/svg/SVGFEConvolveMatrixElement.cpp:117
 +      for (int i = 0; i < numberOfItems; i++)
I'd prefer ++i here, but hey :-)

WebCore/svg/SVGFEConvolveMatrixElement.cpp:145
 +          for (int i = 0; i < numberOfItems; i++)
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:148
 +              divisorValue = 1.0f;
= 1; No .0f postfixes (new style rule).

Please fix before landing! r=me.
------- Comment #23 From 2010-06-29 00:33:06 PST -------
Patch landed in http://trac.webkit.org/changeset/62092, but I plan to upload the second part of the work soon, so I don't close the bug.
------- Comment #24 From 2010-06-29 00:35:12 PST -------
http://trac.webkit.org/changeset/62092 might have broken Qt Linux Release
------- Comment #25 From 2010-06-29 01:00:15 PST -------
http://trac.webkit.org/changeset/62093 might have broken Qt Windows 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62093
http://trac.webkit.org/changeset/62094
http://trac.webkit.org/changeset/62095
------- Comment #26 From 2010-06-29 01:59:23 PST -------
Update expected files landed in:
http://trac.webkit.org/changeset/62096
http://trac.webkit.org/changeset/62097
------- Comment #27 From 2010-06-29 02:00:09 PST -------
(From update of attachment 59890 [details])
remove r+ to remove it from pending queue
------- Comment #28 From 2010-06-30 01:31:31 PST -------
Created an attachment (id=60096) [details]
part 2: the implementation

Unfortunatel I can update only the mac-leopard expected values. I would be happy if someone could help me run this patch on other macs. Otherwise I need to use the build-bot results to update the other macs.
------- Comment #29 From 2010-06-30 01:33:49 PST -------
Created an attachment (id=60097) [details]
part 2: the implementation

Oh, wrong patch before. Sorry.
------- Comment #30 From 2010-06-30 05:12:52 PST -------
(From update of attachment 60097 [details])
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?
------- Comment #31 From 2010-07-01 01:01:49 PST -------
Created an attachment (id=60210) [details]
updated patch
------- Comment #32 From 2010-07-01 01:06:06 PST -------
I did your suggested changes. Thanks for the review.

> 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.

You mean copy the values to a local array? Not sure that will help.

> 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.

Added the assert and a comment. m_divisor cannot be 0, otherwise the filter will be not created.

> 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 :-)

Template arguments must be compile time constants.

I still not have expected pixel test for the generic mac, only for mac-leopard :(
------- Comment #33 From 2010-07-01 01:19:56 PST -------
(In reply to comment #32)
> I did your suggested changes. Thanks for the review.
> 
> > 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.
> 
> You mean copy the values to a local array? Not sure that will help.
Oh, I meant just m_kernelMatrix[kernelValue], as you're calling it multiple times, but it's not worth it - just fine as it is.

> 
> > 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.
> 
> Added the assert and a comment. m_divisor cannot be 0, otherwise the filter will be not created.
Thanks.

> 
> > 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 :-)
> 
> Template arguments must be compile time constants.
Ah right, forgot.

> 
> I still not have expected pixel test for the generic mac, only for mac-leopard :(
Same here, but I don't care about that, as long as we don't have pixel test bots, where I could grab the right results from. Let someone else update them if he/she notices diffs.
I'm just generating mac-leopard results, and put them into mac. If there are already mac-lepard specific results, I'm just regenerating them, and copy the same file into mac/ - until someone else can fix them for me...

Waiting for EWS before giving the final review.
------- Comment #34 From 2010-07-01 02:09:21 PST -------
(From update of attachment 60210 [details])
Great patch, thanks so much! r=me.WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:92
 +      // Only once usage -> can be ALWAYS_INLINE
Just remove this comment, I don't think it helps much.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:96
 +      // Only once usage -> can be ALWAYS_INLINE
Ditto.
------- Comment #35 From 2010-07-01 02:09:45 PST -------
(From update of attachment 60210 [details])
Great patch, thanks so much! r=me.

Please fix some last small issues before landing:

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:92
 +      // Only once usage -> can be ALWAYS_INLINE
Just remove this comment, I don't think it helps much.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:96
 +      // Only once usage -> can be ALWAYS_INLINE
Ditto.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:98
 +      template<bool preserveAlphaValues>
Can you please add a newline between this template<bool> and the line before. Looks less cluttered.
------- Comment #36 From 2010-07-01 02:13:14 PST -------
ALWAYS_INLINE is a hammer we use when the compiler does the wrong thing.  It isn't just to be sprinkled around. :)  We add it for large functions which the compiler refuses to inline on its own.  Just placing something in a class definition (in a header) implicitly marks it "inline".  No need to add ALWAYS_INLINE unless we have demonstrated the compiler fails to comply.  It's better to let the compiler figure things out than to force its hand with ALWAYS_INLINE
------- Comment #37 From 2010-07-01 02:29:21 PST -------
(In reply to comment #36)
> ALWAYS_INLINE is a hammer we use when the compiler does the wrong thing.  It isn't just to be sprinkled around. :)  We add it for large functions which the compiler refuses to inline on its own.  Just placing something in a class definition (in a header) implicitly marks it "inline".  No need to add ALWAYS_INLINE unless we have demonstrated the compiler fails to comply.  It's better to let the compiler figure things out than to force its hand with ALWAYS_INLINE

Well, clearly the function should be inlined. I'd say better safe than sorry - and thus have no problem with the ALWAYS_INLINE usage. It's done like this in the other filter effects (lighting etc.) as well.
------- Comment #38 From 2010-07-01 02:46:11 PST -------
(From update of attachment 60210 [details])
 92     // Only once usage -> can be ALWAYS_INLINE

is not english.
------- Comment #39 From 2010-07-01 02:49:08 PST -------
(In reply to comment #38)
> (From update of attachment 60210 [details] [details])
>  92     // Only once usage -> can be ALWAYS_INLINE
> 
> is not english.

It has been removed already :-)
------- Comment #40 From 2010-07-01 02:52:30 PST -------
> It has been removed already :-)

Yeah, and the patch landed in: http://trac.webkit.org/changeset/62244

If the bots like the patch, I will close the bug.
------- Comment #41 From 2010-07-01 03:14:32 PST -------
Closing bug.