[webkit-reviews] review granted: [Bug 95914] [CSS Shaders] u_textureSize uniform should be set to the size of the texture. : [Attachment 163733] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Michelangelo De Simone
<michelangelo at webkit.org>'s request for review:
Bug 95914: [CSS Shaders] u_textureSize uniform should be set to the size of the
texture.
https://bugs.webkit.org/show_bug.cgi?id=95914

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=163733&action=review


> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:357
> +    if (m_compiledProgram->meshSizeLocation() != -1)
> +	   m_context->uniform2f(m_compiledProgram->meshSizeLocation(),
m_meshColumns, m_meshRows);
> +
> +    ASSERT(m_meshColumns);
> +    ASSERT(m_meshRows);

Might as well put the ASSERT before this first binding.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:365
> +    if (m_compiledProgram->meshBoxLocation() != -1)
> +	   // FIXME: This will change when filter margins will be implemented,
> +	   // see https://bugs.webkit.org/show_bug.cgi?id=71400
> +	   m_context->uniform4f(m_compiledProgram->meshBoxLocation(), -0.5,
-0.5, 1.0, 1.0);

Please enclose in {} if you're putting comments before the statement, or put
the comment before the if.

> LayoutTests/css3/filters/resources/u-mesh-box.fs:1
> +// Test whether u_meshBox is correctly set; change fragment color to green
if yes.

Could you rename these files (u-mesh-box.fs u-mesh-size, u-texture-size,
u-tile-size) to reflect the parameters they expect? I think people often look
through layout tests to find examples of code, and might interpret this to be
such. In reality it is u-mesh-box-is-unit-square or something. Do you agree?