[webkit-reviews] review denied: [Bug 78720] [Qt] Use GraphicsContext3DOpenGLES.cpp when using OpenGL ES : [Attachment 133473] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Roland Takacs
<rtakacs at inf.u-szeged.hu>'s request for review:
Bug 78720: [Qt] Use GraphicsContext3DOpenGLES.cpp when using OpenGL ES
https://bugs.webkit.org/show_bug.cgi?id=78720

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

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


Good work, see comments.

> Source/WebCore/ChangeLog:9
> +	   No new tests, just a buildfix.
> +

I wouldn't call this a build fix. I'd say instead that it's tested by existing
WebGL tests.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:947
> +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2)

I e should add
#if defined(QT_OPENGL_ES_2)
#define WTF_USE_OPENGL_ES_2 1
#endif

to Platform.h instead.
Then we don't need to use ||.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:173

> +    ::glBindFramebuffer(GL_FRAMEBUFFER, m_fbo);

Wouldn't this break some existing desktop stuff, where EXT is needed?
I'd rather this be moved to a bindFramebuffer that's implemented differently in
ES and desktop.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:212

> +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2)

I'd rather we move all the ES specific functionality into the ES/desktop files,
rather than have #ifdefs inside common. This applies to a lot of the following
changes :)