[Webkit-unassigned] [Bug 5861] feConvolveMatrix filter is not implemented

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=5861


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58758|review?                     |review-
               Flag|                            |




--- Comment #14 from Nikolas Zimmermann <zimmermann at kde.org>  2010-06-23 00:46:13 PST ---
(From update of attachment 58758)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.