[webkit-reviews] review denied: [Bug 105602] [EFL][WebGL] Implement EGL support with GLX : [Attachment 180633] patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Kalyan
<kalyan.kondapally at intel.com>'s request for review:
Bug 105602: [EFL][WebGL] Implement EGL support with GLX
https://bugs.webkit.org/show_bug.cgi?id=105602

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

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


Have to sort out the m_contextHandle stuff...

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:89
> +    OwnPtr<GLPlatformContext> context = adoptPtr(new
GLXCurrentContextWrapper());
> +    return context.release();

You don't need two lines here, adoptPtr already returns a PassOwnPtr.

return adoptPtr(new GLXCurrentContextWrapper());

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:92
> +    OwnPtr<GLPlatformContext> context = adoptPtr(new
EGLCurrentContextWrapper());
> +    return context.release();

ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:116
> +#if USE(OPENGL_ES_2)
> +	       EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> +#else
> +	       EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> +#endif

Instead of repeating this twice, you can have a function EGLint
getRenderableType() that returns a different value based on the define.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:140
> +#if USE(OPENGL_ES_2)
> +	       EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> +#else
> +	       EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> +#endif

Ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:29
> +#if USE(ACCELERATED_COMPOSITING) && USE(EGL)

I don't understand why all these classes are wrapped with
ACCELERATED_COMPOSITING.

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:57
> +    static bool queryDone = false;

queryDone => didQueryForRobustnessExtension

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:58
> +    static bool isEGLEXTsupported = false;

isRobustnessExtensionSupported

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:77
> +// FIXME: This is a temporary workaround till we are able to build evas with
EGL support

till => until
Period at end of sentence(.)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:111
> +	   if (!m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)

EGL_NO_CONTEXT is 0
So this line means if (!handle && handle)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:116
> +	   if (m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)

This is superfluously redundant :)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:40
> +    PlatformContext handle() const;

add OVERRIDE keyword for overrides