Currently, we set GRAPHICS_SURFACE to true if support for XCompositeWindow is identified during compile time. This is ok, when using GLX. With EGL and GLES2.0 we want to use XPixmap or some kind of Drm Buffer(if supported) instead of XCompositeWindow. We could support both Window and Pixmap for EGL but I think it is unnecessary overhead and we could just use XPixmap for both desktop and when using GLES2.0. Thus, it would be be better to introduce a new macro to identify XCompsiteWindow support and alwayse set GraphicsSurface.
Created attachment 190030 [details] Patch
Comment on attachment 190030 [details] Patch 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.
updated the bug title to reflect the changes.
Created attachment 190879 [details] patchv2
(In reply to comment #2) > (From update of attachment 190030 [details]) > 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 would prefer to change it to USE as even though we have the support for GLX we dont use it if one builds with EGL support during compile time. This would also sync in with other places with similar checks(i.e our own code and also on GTK side). Any thoughts? > > Let's delay adding the macro until we have code supporting the !USE(XCOMPOSITE) case. Done.
Comment on attachment 190879 [details] patchv2 Attachment 190879 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16763197 New failing tests: editing/selection/selection-invalid-offset.html
(In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 190030 [details] [details]) > > 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 would prefer to change it to USE as even though we have the support for GLX we dont use it if one builds with EGL support during compile time. This would also sync in with other places with similar checks(i.e our own code and also on GTK side). Any thoughts? Ok, I have not realized that we're already using _both_ HAVE(GLX) and USE(GLX) in the code. Standardizing on USE(GLX) make sense to me.
Comment on attachment 190879 [details] patchv2 r=me. We should move X11Helper our form the glx directory as well, I find it a bit confusing. Can you file a bug for that ?
Created attachment 191156 [details] patchforlanding
Comment on attachment 191156 [details] patchforlanding Clearing flags on attachment: 191156 Committed r144602: <http://trac.webkit.org/changeset/144602>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > (From update of attachment 190879 [details]) > r=me. > > We should move X11Helper our form the glx directory as well, I find it a bit confusing. Can you file a bug for that ? Created a new issue for this: https://bugs.webkit.org/show_bug.cgi?id=111347