[webkit-reviews] review denied: [Bug 106582] [EFL] [WebGL]Add error handling to carefully manage Window backing pixmaps. : [Attachment 182344] patchv3

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Kalyan
<kalyan.kondapally at intel.com>'s request for review:
Bug 106582: [EFL] [WebGL]Add error handling to carefully manage Window backing
pixmaps.
https://bugs.webkit.org/show_bug.cgi?id=106582

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182344&action=review


would like someone else to have a look

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:79
> +class ScopedErrorHandler {

I would add an X here... like ScopedXErrorHandler... also as it only treads
some errors, it should maybe be renamed.

like ScopedXWindowCreationErrorHandler or so

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:93
> +	   XSetErrorHandler(m_originalErrHandler);

I dont like Err... I would just called it m_previousHandler here

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:278
> +	   // Ensure that the window is mapped.
> +	   if (attr.map_state == IsUnmapped || attr.map_state == IsUnviewable)
> +	       return;

these changes are not explained in the changelog

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:420
> +    void reset()

this seems more like a clear() than a reset.. as it doesnt set anything again

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:539
> +
> +    if (m_private->isReceiver() && platformGetTextureID()) {
>	   glBindTexture(GL_TEXTURE_2D, platformGetTextureID());

these changes should be explained

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:553
> +    return m_private ? m_private->size() : IntSize();

ditto

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:93
>  
>      XSetWindowBackgroundPixmap(display, tempHandleId, 0);
> +#if USE(GRAPHICS_SURFACE)
>      XCompositeRedirectWindow(display, tempHandleId,
CompositeRedirectManual);
> +#endif
>      *handleId = tempHandleId;

no error handling needed herE?

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:157
> +	   for (int i = 0; i< matchingCount; i++) {

space after i please. change needs to be explained