[webkit-reviews] review denied: [Bug 113359] [EFL][EGL] Add support for creating offscreen surface. : [Attachment 195381] patchv2

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Kalyan
<kalyan.kondapally at intel.com>'s request for review:
Bug 113359: [EFL][EGL] Add support for creating offscreen surface.
https://bugs.webkit.org/show_bug.cgi?id=113359

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

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


> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:66
>	   configAttributeList[11] = m_attributes &
GLPlatformSurface::SupportAlpha ? 8 : 0;
>	   configAttributeList[13] = EGL_PIXMAP_BIT;

I know this code was there before, but this is extremely bug prone. I'd prefer
if instead of using a static array and changing it here with a magic index
you'd just generate the array here on the fly.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:77
>	   configAttributeList[11] = m_attributes &
GLPlatformSurface::SupportAlpha ? 8 : 0;
>	   configAttributeList[13] = EGL_WINDOW_BIT;

ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:112
> +    EGLint numConfigs, i;

Two lines

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:119
> +	   return config;

return 0;

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:122
> +    configs = static_cast<EGLConfig*>(malloc(numConfigs *
sizeof(EGLConfig)));

configs = new EGLConfig[numConfigs];
C++ please :)

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:163
> +	   if (alpha != configAttributeList[11])
> +	       continue;
> +
> +	   eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_RED_SIZE, &red);

> +
> +	   if (red != configAttributeList[3])
> +	       continue;
> +
> +	   eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_GREEN_SIZE,
&green);
> +
> +	   if (green != configAttributeList[5])
> +	       continue;
> +
> +	   eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_BLUE_SIZE,
&blue);
> +
> +	   if (blue != configAttributeList[7])
> +	       continue;
> +
> +	   eglGetConfigAttrib(m_sharedDisplay, tempConfig, EGL_SURFACE_TYPE,
&surfaces);
> +
> +	   if (surfaces & configAttributeList[13]) {

Please no magic indices.
You should have something like expectedAlpha, expectedGreen etc. and put them
dynamically into the array.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigSelector.cpp:172
> +    if ((m_attributes & GLPlatformSurface::SupportAlpha) && !config) {
> +	   m_attributes &= ~GLPlatformSurface::SupportAlpha;
> +	   config = configs[0];
> +    }

I don't quite get what the logic here is.

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:31
> +#include <glx/X11Helper.h>

EGLHelper includes something from GLX?

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:36
> +typedef X11Helper NativeWrapper;

Seems like this should be guarded with X11

> Source/WebCore/platform/graphics/surfaces/egl/EGLHelper.h:41
> +

No need for new line

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:179
> +    m_drawable = eglCreatePixmapSurface(m_sharedDisplay, config,
(EGLNativePixmapType)m_bufferHandle, 0);

No C casting please

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:62
> +

Unnecessary line

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:64
> +    EGLPixmapSurface(SurfaceAttributes);

explicit