[webkit-reviews] review denied: [Bug 78674] [Texmap] Better management of shaders in TextureMapperGL : [Attachment 127848] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 78674: [Texmap] Better management of shaders in TextureMapperGL
https://bugs.webkit.org/show_bug.cgi?id=78674

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

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=127848&action=review


> Source/WebCore/ChangeLog:8
> +	   Split ShaderMapperGL.cpp into two files.

You mean TextureMapperGL?

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.h:49
> +    enum TextureTransparency {
> +	   Opaque,
> +	   Transparent
> +    };
> +
> +    enum Flip {
> +	   ShouldFlip,
> +	   NoFlip
> +    };

Just make this into one enum called Flag with
0x01 = SupportsBlending,
0x02 = ShouldFlipTexture

and then pass it as an integer mask.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:23
> +#if USE(ACCELERATED_COMPOSITING)

&& USE(TEXTURE_MAPPER)

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.cpp:53
> +const char* fragmentShaderSourceOpacityAndMask =

These should all be declared static, otherwise they take up symbols.

> Source/WebCore/platform/graphics/opengl/TextureMapperShaderManager.h:54
> +inline static void debugGLCommand(const char* command, int line)
> +{
> +    const GLenum err = glGetError();
> +    if (!err)
> +	   return;
> +    WTFReportError(__FILE__, line, WTF_PRETTY_FUNCTION, "[TextureMapper GL]
Command failed: %s (%x)\n", command, err);
> +    ASSERT_NOT_REACHED();
> +}
> +

I don't like this here. It should be inside TextureMapperGL.cpp, and we should
simply not use GL_CMD from inside TextureMapperShaderManager.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:82
> +    inline bool isOpaque() const { return m_flags ^ SupportsAlpha; }

^ ??