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

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 77921: [GTK] Add support for creating EGL contexts
https://bugs.webkit.org/show_bug.cgi?id=77921

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136701&action=review


> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:51
> +{

We should add ASSERT(!sharedDisplay()) here and/or strengthen the return value
handling of eglChooseConfig() below.

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:53
> +    // TODO: We assume that we're creating an OpenGLES2 context for now,
> +    // but later it would be better to do something smarter here.

Nit: This should be a FIXME comment per the WebKit Code Style Guidelines,
<http://www.webkit.org/coding/coding-style.html#comments-fixme>.

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:71
> +    EGLint numberConfigsReturned;
> +    return eglChooseConfig(sharedDisplay(), attributeList, config, 1,
&numberConfigsReturned) && numberConfigsReturned;

How does this code work when eglChooseConfig() returns an error code?

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:134
> +    static bool initialized = false;
> +    static bool success = true;
> +    if (!initialized) {
> +	   success = initializeOpenGLShims();
> +	   initialized = true;
> +    }
> +    if (!success)
> +	   return 0;

I would write this as:

static bool didInitializeOpenGLShims = initializeOpenGLShims();
if (!didInitializeOpenGLShims)
    return 0;

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:174
> +    if (eglGetCurrentContext() == m_context)
> +	   return true;
> +    return eglMakeCurrent(sharedDisplay(), m_surface, m_surface, m_context);


I would write this as:

return eglGetCurrentContext() == m_context || eglMakeCurrent(sharedDisplay(),
m_surface, m_surface, m_context);

> Source/WebCore/platform/graphics/gtk/GLContextGtk.cpp:69
> +	   context =  GLContextEGL::createContext(GDK_WINDOW_XID(gdkWindow));

Nit: There are two spaces to the right of the equal sign (=).

> Source/WebCore/platform/graphics/gtk/GLContextGtk.cpp:74
> +	       context = 
GLContextGLX::createContext(GDK_WINDOW_XID(gdkWindow));

Nit: There are two spaces to the right of the equal sign (=).

Do you expect most people to build with EGL and GLX enabled by default? If so,
how often do you expect GLContextEGL::createContext() to return 0. Should we
cache GDK_WINDOW_XID(gdkWindow) in a local variable? Notice, GDK_WINDOW_XID
deferences a pointer twice,
<http://developer.gnome.org/gdk/stable/gdk-X-Window-System-Interaction.html#GDK
-WINDOW-XID:CAPS>.