Bug 75598 - [Qt][Texmap] Convert shaders in TextureMapperGL to use a macro
Summary: [Qt][Texmap] Convert shaders in TextureMapperGL to use a macro
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
Depends on:
Reported: 2012-01-04 18:55 PST by Noam Rosenthal
Modified: 2012-01-05 17:29 PST (History)
4 users (show)

See Also:

Patch (7.24 KB, patch)
2012-01-04 19:01 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2012-01-05 13:18 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2012-01-05 15:03 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-04 18:55:08 PST
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.
Comment 1 Noam Rosenthal 2012-01-04 19:01:31 PST
Created attachment 121205 [details]
Comment 2 Noam Rosenthal 2012-01-05 13:18:26 PST
Created attachment 121319 [details]
Comment 3 Noam Rosenthal 2012-01-05 13:19:53 PST
Had to make some changes in order to apply the patch;
I feel that those changes warrant a re-review.
Comment 4 Martin Robinson 2012-01-05 14:41:28 PST
Comment on attachment 121319 [details]

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

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.
Comment 5 Noam Rosenthal 2012-01-05 15:03:26 PST
Created attachment 121339 [details]
Comment 6 WebKit Review Bot 2012-01-05 17:29:32 PST
Comment on attachment 121339 [details]

Clearing flags on attachment: 121339

Committed r104255: <http://trac.webkit.org/changeset/104255>
Comment 7 WebKit Review Bot 2012-01-05 17:29:37 PST
All reviewed patches have been landed.  Closing bug.