[webkit-reviews] review granted: [Bug 75598] [Qt][Texmap] Convert shaders in TextureMapperGL to use a macro : [Attachment 121319] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 75598: [Qt][Texmap] Convert shaders in TextureMapperGL to use a macro
https://bugs.webkit.org/show_bug.cgi?id=75598

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121319&action=review


This looks good to me. There are some style nits below, but feel free to fix
them before landing.

By the way, the TextureMapper seems to be working really well for the GTK+ port
so far!

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:321
> +#define VERTEX_SHADER(Src...) OES2_PRECISION_DEFINITIONS#Src
> +#define FRAGMENT_SHADER(Src...) OES2_PRECISION_DEFINITIONS\

I like how you've hidden the details of OES here. In many of the other macros
with parameters I see in WebKit, the parameter name follows WebKit naming
conventions. So in this case, I prefer src over Src, unless there is some other
compelling reason to use the capitalized variant.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:331
> +	       void main(void)
> +	       {

Similarly, I think the placement of curly braces in shader source should also
follow WebKit C++/C/JavaScript style.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:334
> +		   lowp float o = Opacity * maskColor.a;

I'd prefer a name like fragmentAlpha or alpha instead of o.