[webkit-reviews] review denied: [Bug 78672] [Texmap] Move TextureMapperGL to use GraphicsContext3D : [Attachment 153657] Try fix Qt4 build

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Helder Correia
<helder.correia at nokia.com>'s request for review:
Bug 78672: [Texmap] Move TextureMapperGL to use GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=78672

Attachment 153657: Try fix Qt4 build
https://bugs.webkit.org/attachment.cgi?id=153657&action=review

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


Thanks for working on this laborious patch!
See comments.

> Source/WebCore/ChangeLog:10
> +	   TextureMapperGL (TMGL) includes direct GL calls and
> +	   GraphicsContext3D (GC3D) offers many conveniences over the
> +	   former approach.  This patch changes the backend of TMGL to

For example, using existing CSS shader code, ANGLE for shader compilation,
reusing WebCore::Texture, having shaders and textures that can delete
themselves.

> Source/WebCore/ChangeLog:13
> +	   TextureMapperGC3D), but in the end it was agreed to consolidate
> +	   it inside TMGL instead.

The rationale is that an additional TextureMapper subclass would get stale very
fast.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:127
> +	   // In this case, we are not setting up WebGL, but rather
TextureMapperGL, and want to rely

We might use it for other things than TextureMapperGL.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:376
> +	   // In this case, we are not setting up WebGL, but rather
TextureMapperGL, hence skip all
> +	   // canvas and ANGLE logic.

We shouldn't skip the ANGLE logic.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:43
> +#if OS(DARWIN)

I think we can move all the OS(DARWIN) stuff under !USE(TEXMAP_OPENGL_ES_2),
apart from defining TRANSFER_TYPE to INT_8_8_8...

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:52
> +#if !defined(TEXMAP_OPENGL_ES_2)
> +#define GL_UNPACK_ROW_LENGTH 0x0CF2
> +#define GL_UNPACK_SKIP_PIXELS 0x0CF4
> +#define GL_UNPACK_SKIP_ROWS 0x0CF3
>  #endif

Add a FIXME: Move this to Extensions3D

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:200
> +    m_context3D = GraphicsContext3D::create(attributes, 0,
GraphicsContext3D::RenderDirectlyToHostWindow);

Add a /* ... */ comment next to the magical 0

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:201
> +    m_data = new TextureMapperGLData(this);

You don't really use anything from TextureMapper other than the gc3d... why
don't we just pass that as a parameter?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:287
> +    const GC3Dfloat m4[] = {
>	   matrix.m11(), matrix.m12(), matrix.m13(), matrix.m14(),
>	   matrix.m21(), matrix.m22(), matrix.m23(), matrix.m24(),
>	   matrix.m31(), matrix.m32(), matrix.m33(), matrix.m34(),
>	   matrix.m41(), matrix.m42(), matrix.m43(), matrix.m44()
>      };
> -    GL_CMD(glUniformMatrix4fv(shaderProgram->matrixLocation(), 1, GL_FALSE,
m4));
> +    m_context3D->uniformMatrix4fv(shaderProgram->matrixLocation(), 1, false,
const_cast<GC3Dfloat*>(m4));

Instead of const_cast, why don't you make the array non-const to beginw ith?