[webkit-reviews] review denied: [Bug 108034] [EFL][WebGL] Define new macro to identify support for XCompositeWindow : [Attachment 190030] Patch

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.gombos at webkit.org> has denied Kalyan
<kalyan.kondapally at intel.com>'s request for review:
Bug 108034: [EFL][WebGL] Define new macro to identify support for
XCompositeWindow
https://bugs.webkit.org/show_bug.cgi?id=108034

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

------- Additional Comments from Laszlo Gombos <laszlo.gombos at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190030&action=review


For now we should assume XCompositeWindow for USE(GLX). I would not yet expose
the guard for XCOMPOSITE to the common code.

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:31
> -#if HAVE(GLX)
> +#if USE(GLX)

I do not think the name change here is necessary. I would remove this change.

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigSelector.h:154
> -#if USE(GRAPHICS_SURFACE)
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

I would just use USE(GRAPHICS_SURFACE) && USE(GLX) as a guard and set
WTF_USE_GLX for Qt (perhaps in Platform.h or in the Qt build system).

> Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:186
> -#if USE(GRAPHICS_SURFACE)
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

Ditto.

> Source/WebCore/platform/graphics/surfaces/glx/X11Helper.h:36
> +#if USE(XCOMPOSITE) || (PLATFORM(QT) && USE(GRAPHICS_SURFACE))

USE(GLX)

> Source/cmake/OptionsEfl.cmake:189
> +	   set(USE_GRAPHICS_SURFACE 1)

I think we should move this rule out from the build system into Platform.h.

> Source/cmake/OptionsEfl.cmake:205
> +		set(USE_GRAPHICS_SURFACE 1)

Ditto we should remove this line and move the rule to Platform.h. Would the
following work in Platform.h ? 

#if (USE(EGL) && PLATFORM(EFL)) || (USE(GLX) && (PLATFORM(EFL) ||
PLATFORM(QT)))
#define WTF_USE_GRAPHICS_SURFACE 1
#endif

Fail the build if there is no Xcomposite and add a FIXME to add support for it
later.

# FIXME: Add support for NOT X11_Xcomposite for GLX
if (NOT X11_Xcomposite_FOUND OR NOT X11_Xrender_FOUND)
message( FATAL_ERROR "GLX support requires X11_Xcomposite.")
endif ()

> ChangeLog:8
> +	   Add a macro to identify XComposite ext support.

Let's delay adding the macro until we have code supporting the !USE(XCOMPOSITE)
case.