Bug 75598 – [Qt][Texmap] Convert shaders in TextureMapperGL to use a macro

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

The literal string representation with quotes makes it hard to read and add shader code.
A SHADER macro like in Chromium is much more suitable and less uglified.

[Noam Rosenthal]

Created attachment 121205 [details]
Patch

[Noam Rosenthal]

Created attachment 121319 [details]
Patch

[Noam Rosenthal]

Had to make some changes in order to apply the patch;
I feel that those changes warrant a re-review.

[Martin Robinson]

Comment on attachment 121319 [details]
Patch

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.

[Noam Rosenthal]

Created attachment 121339 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121339 [details]
Patch

Clearing flags on attachment: 121339

Committed r104255: <http://trac.webkit.org/changeset/104255>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.