[webkit-reviews] review denied: [Bug 5864] feTurbulence is not implemented : [Attachment 62688] SVG feTurbulence imeplementation

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Renata Hodovan
<hodovan at inf.u-szeged.hu>'s request for review:
Bug 5864: feTurbulence is not implemented
https://bugs.webkit.org/show_bug.cgi?id=5864

Attachment 62688: SVG feTurbulence imeplementation 
https://bugs.webkit.org/attachment.cgi?id=62688&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Some issues from the last review are still present:

1) The "No new tests" statment is probably wrong, as it's covered by at least
the existing W3C SVG tests. It probably also changes other test results, did
you run the layout tests? It's necessary to run them on a Mac, to produce new
pixel test results (ask Zoltan, who knows about the procedure)

2) WebCore/svg/graphics/filters/SVGFETurbulence.h:41
 +  static PassRefPtr<FETurbulence> create(TurbulanceType, float, float, int,
float, bool);
The indention is not correct, you either have to change the whole file, or
leave at is - mixed indention is not correct :-)

And some new ones:

WebCore/svg/graphics/filters/SVGFETurbulence.h:5
 +		      2005 Dirk Schulze <krit at webkit.org>
I guess you meant 2009 here?

WebCore/svg/graphics/filters/SVGFETurbulence.h:66
 +	static const int blockSize = 256;
static variables need a s_ prefix, so it should be s_blockSize.

WebCore/svg/graphics/filters/SVGFETurbulence.h:67
 +	static const int blockMask = blockSize - 1;
Ditto, should be named s_blockMask.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:46
 +  static const int perlinNoise = 4096;
All these statics need the s_ prefix.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:53
 +	int numOctaves, float seed, bool stitchTiles)
No need to wrap lines here, just make a long line.
WebCore/svg/graphics/filters/SVGFETurbulence.cpp:162
 +	// The seed value clamp to the range 1 to (randMaximum - 1).
The seed value is clamped to the range [1, randMaximum - 1].

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:208
 +	    int b;
These are still not clear to me.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:209
 +	    float r;
Ditto. Really needs a better name :(

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:238
 +	int tmp = paintingData.latticeSelector[latticeIndex + noiseY.b];
Maybe add a comment here that this is 1:1 taken from the SVG spec, with another
reference to http://www.w3.org/TR/SVG11/filters.html#feTurbulenceElement.
Rename tmp to temp at least :-)

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:255
 +  unsigned char FETurbulence::calculateTurbulenceValueForPoint(PaintingData&
paintingData, FloatPoint point)
FloatPoint should be passed as const reference: const FloatPoint& point, to
avoid copying.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:257
 +	float tileWidth = paintingData.filterSize.width();
I see that you're early returning in another place when the width() is zero.
Still add an assertion here for tileWidth > 0, as it makes the code easier to
read.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:258
 +	float tileHeight = paintingData.filterSize.height();
Ditto.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:267
 +		ASSERT(m_baseFrequencyX >= 0);
Ok this is unncessary as you said on IRC, as you're checking if
(m_baseFrequencyX) before.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:276
 +		ASSERT(m_baseFrequencyY >= 0);
Ditto.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:282
 +	    // Set up TurbulenceInitial stitch values. It's cast to the higher
interer value.
s/interere/integer/.
I'd really appreciate if we'd use roundf though, as these hacks are harder to
maintain, we're not using this + 0.5 tricks in other places, but round.
As last resort, you could add a comment that the SVG spec pseudo-code does it
this way, and you're just following it for the sake of compatibility.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:290
 +	float turbFunctionResult = 0;
s/turbFunction/turbulenceFunction/.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:312
 +	turbFunctionResult = std::max(std::min(turbFunctionResult,
static_cast<float>(255)), static_cast<float>(0));
maybe use 255.f here, and 0.f instead of casting to float.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:314
 +	    return static_cast<unsigned char>(turbFunctionResult * 127.5f +
127.5f); // It comes form (turbFunctionResult * 255 + 255) / 2
s/form/from.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:322
 +	if (!resultImage()->size().width() || !resultImage()->size().height())
I'd optimize for the common case:
Swap the order, first:
IntRect imageRect(IntPoint(), resultImage()->size());
then:
if (!imageRect.size().width() || !imageRect.size().height())
    return;

instead of calling resultImage()->size() multiple times.


Looking forward to the next patch.