[webkit-reviews] review denied: [Bug 77921] [GTK] Add support for creating EGL contexts : [Attachment 165600] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied José Dapena Paz
<jdapena at igalia.com>'s request for review:
Bug 77921: [GTK] Add support for creating EGL contexts
https://bugs.webkit.org/show_bug.cgi?id=77921

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165600&action=review


Looks good! Just a few minor thing. I'm incredibly sorry that it took me so
long to review this patch. :(

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:33
> +#if USE(OPENGL_ES_2)
> +#include "OpenGLESShims.h"
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +#else
>  #include "OpenGLShims.h"
> +#endif

These blocks of includes should be after the main blocks like this:

#include "HostWindow.h"
#include "NotImplemented.h"
include "PlatformContextCairo.h"
#include <wtf/OwnArrayPtr.h>

#if USE(OPENGL_ES_2)
#include "OpenGLESShims.h"
#include <GLES2/gl2.h>
#include <GLES2/gl2ext.h>
#else
#include "OpenGLShims.h"
#endif

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:39
> +static const EGLenum gGLApi = EGL_OPENGL_ES_API;

Nit: This should be called gGLAPI

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:82
> +void GLContextEGL::addActiveContext(GLContextEGL* context)
> +{
> +    static bool addedAtExitHandler = false;
> +    if (!addedAtExitHandler) {
> +	   atexit(&GLContextEGL::cleanupSharedEGLDisplay);
> +	   addedAtExitHandler = true;
> +    }
> +    GLContext::addActiveContext(context);
> +}

I think this work-around could possibly be removed for EGL. That would make
this patch quite a bit smaller. Did you find that it prevented crashes? It's
probably okay to let the display leak here.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:109
> +    if (surfaceType == GLContextEGL::PbufferSurface) {
> +	   attributeList[10] = EGL_SURFACE_TYPE;
> +	   attributeList[11] = EGL_PBUFFER_BIT;
> +    } else if (surfaceType == GLContextEGL::PixmapSurface) {
> +	   attributeList[10] = EGL_SURFACE_TYPE;
> +	   attributeList[11] = EGL_PIXMAP_BIT;
> +    } else if (surfaceType == GLContextEGL::WindowSurface) {
> +	   attributeList[10] = EGL_SURFACE_TYPE;
> +	   attributeList[11] = EGL_WINDOW_BIT;

It looks like you could set attributeList[10] when you create attributeList
above and then do a switch statement to set the actual surface type. A switch
statement is important here because it will make the compiler emit a warning if
you added a new EGLSurfaceType and didn't handle it, in the future.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:242
> +    removeActiveContext(this);

Assuming the patch continues to keep a list of contexts for cleaning up, why
not move this call to removeActiveContext and the one for addActiveContext
above to the superclass constructor and destructor?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:277
> +    if (m_surface)
> +	   eglSwapBuffers(sharedEGLDisplay(), m_surface);

It seems that you always have a surface, so this should be:

ASSERT(m_surface);
eglSwapBuffers(sharedEGLDisplay(), m_surface);

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:53
> -    Display* display = GLContextGLX::sharedDisplay();
> +    Display* display = GLContext::sharedX11Display();

I think this has changed now, so you may need to rebase. Sorry. :(