Decouple native window management and logic to find surface configuration. This would align GLX implementation with EGL.
Created attachment 180862 [details] patch
Comment on attachment 180862 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=180862&action=review > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:81 > + GLXFBConfig surfaceContextConfig(bool supportsXRenderExtension) wouldn't an enum be better? > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:111 > + { > + if (m_pbufferFBConfig) > + m_pbufferFBConfig = 0; > + > + if (m_surfaceContextFBConfig) > + m_surfaceContextFBConfig = 0; > + } why not just set them always to 0 > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:51 > + OwnPtr<GLXConfigHelper> m_ConfigHelper; m_conf... not uppercase C
Created attachment 180865 [details] patchv2
(In reply to comment #2) > (From update of attachment 180862 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180862&action=review > > > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:81 > > + GLXFBConfig surfaceContextConfig(bool supportsXRenderExtension) > > wouldn't an enum be better? I made changes so that the value is passed during construction of confighelper, as this value doesn't change during run time. We could probably go for enum in future if we need to track more than one extension support. Any thoughts ?? > why not just set them always to 0 done > > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:51 > > + OwnPtr<GLXConfigHelper> m_ConfigHelper; > > m_conf... not uppercase C done
Comment on attachment 180865 [details] patchv2 View in context: https://bugs.webkit.org/attachment.cgi?id=180865&action=review > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:35 > +class GLXConfigHelper { Helper is a bad name... it makes it difficult to understand what its responsibility is > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:42 > + , m_VisualInfo(0) no, not uppercase V! We should fix the style checker to catch this, can you file a bug for that? > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:51 > + virtual ~GLXConfigHelper() > + { > + reset(); > + } reset() doesnt seem needed... it is the dtor! > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:72 > + int numReturned; numConfigurations would make more sense, or numAvailableConfigs > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:114 > + int numReturned; same > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:139 > + } > + } else if (m_VisualInfo->depth == 32) { > +#else > + if (m_VisualInfo->depth == 32) { > +#endif something is wrong with indentation here! > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:41 > + m_sharedDisplay = m_nativeResource->nativeSharedDisplay(); > + if (!m_sharedDisplay) newline before if
Created attachment 180939 [details] patchv3
(In reply to comment #5) > (From update of attachment 180865 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180865&action=review > > Helper is a bad name... it makes it difficult to understand what its responsibility is changed it to GLXConfigSelector > > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:42 > > + , m_VisualInfo(0) > > no, not uppercase V! We should fix the style checker to catch this, can you file a bug for that? fixed, k I will file a bug for it. > > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:51 > > + virtual ~GLXConfigHelper() > > + { > > + reset(); > > + } > > reset() doesnt seem needed... it is the dtor! ya, removed it. > > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:72 > > + int numReturned; > same done.
Comment on attachment 180939 [details] patchv3 Clearing flags on attachment: 180939 Committed r138567: <http://trac.webkit.org/changeset/138567>
All reviewed patches have been landed. Closing bug.