[webkit-reviews] review denied: [Bug 5861] feConvolveMatrix filter is not implemented : [Attachment 58758] next try

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 5861: feConvolveMatrix filter is not implemented
https://bugs.webkit.org/show_bug.cgi?id=5861

Attachment 58758: next try
https://bugs.webkit.org/attachment.cgi?id=58758&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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!