[webkit-reviews] review denied: [Bug 73317] [CSS Shaders] Add FECustomFilter that renders custom filters : [Attachment 119450] Patch V1

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 73317: [CSS Shaders] Add FECustomFilter that renders custom filters
https://bugs.webkit.org/show_bug.cgi?id=73317

Attachment 119450: Patch V1
https://bugs.webkit.org/attachment.cgi?id=119450&action=review

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119450&action=review


Please make another pass, trying to simplify the mesh manipulation logic

> LayoutTests/css3/filters/resources/fragment.shader:1
> +precision mediump float;

I don't like the .shader suffix much. It would be more descriptive if you used
.vs and .fs, which are fairly standard shader suffixes.

> LayoutTests/platform/wk2/Skipped:192
> +css3/filters/custom-filter.html

Please mention why you had to turn off this test in wk2 up in your ChangeLog.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:98
> +		   vertices.append(hSize * i - 0.5f + meshBoxX);

Factor out common code between this and addPoint()

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:109
> +	   #define ADD_POINT(x, y) \

This is confusingly called ADD_POINT, when it doesn't use addPoint(), and has
the same name as the ADD_POINT below, which does. Seems like all this vertex
list manipulation logic could be coalesced into a small set of functions.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:51
> +    unsigned verticesBuffer() const { return m_verticesBuffer; }

I can't tell what verticesBuffer is from the name or return type. I know it is
a GC3D buffer object, so it's type should be Platform3DObject. And maybe it
could be called verticesBufferObject.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:54
> +    unsigned elementsBuffer() const { return m_elementsBuffer; }

Ditto

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1201
> +void TransformationMatrix::toColumnMajorFloatArray(float* result) const

I think we're actually storing the matrix in column major order (if not, we
should be). So it seems like this could be a memcpy()

> Source/WebCore/rendering/FilterEffectRenderer.cpp:242
> +#if ENABLE(WEBGL)

Shouldn't this be ENABLE(WEBGL) && ENABLE(CSS_SHADERS). There's no need to have
this code compiled in if CSS_SHADERS is off, is there?