[webkit-reviews] review denied: [Bug 73317] [CSS Shaders] Add FECustomFilter that renders custom filters : [Attachment 119450] Patch V1
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 15 16:09:17 PST 2011
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?
More information about the webkit-reviews
mailing list