In our side, we check if GLX is supported or not. We should change this check to USE, to be able to configure using EGL or GLX
Created attachment 180208 [details] patch
Comment on attachment 180208 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=180208&action=review I think renaming from HAVE() to USE() make sense. I only reviewed the EFL-only parts (OptionsEfl.cmake) to make the next round of review easier. r- to fix the EFL CMake file and make it consistent with the intention of the patch. > Source/cmake/OptionsEfl.cmake:170 > add_definitions(-DHAVE_GLX) I think this can be removed now. > Source/cmake/OptionsEfl.cmake:171 > + set(WTF_USE_GLX 1) This is not needed - WTF_USE_GLX variable is not used anywhere in the make system so no need to set it. > Source/cmake/OptionsEfl.cmake:172 > + add_definitions(-DWTF_USE_GLX=1) This is correct, this is the only definition that actually needed. > Source/cmake/OptionsEfl.cmake:174 > + set(ENABLE_GLX 1) > + add_definitions(-DENABLE_GLX=1) Not needed, not used anywhere.
Created attachment 180231 [details] patchv2 review fixes
(In reply to comment #2) > (From update of attachment 180208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180208&action=review > > > Source/cmake/OptionsEfl.cmake:174 > > + set(ENABLE_GLX 1) > > + add_definitions(-DENABLE_GLX=1) > > Not needed, not used anywhere. Done.
Comment on attachment 180231 [details] patchv2 View in context: https://bugs.webkit.org/attachment.cgi?id=180231&action=review This looks like something which should be two patches > ChangeLog:8 > + This patch changes the check Have(GLX) to Use(GLX). HAVE, USE! > Source/WebCore/platform/graphics/opengl/GLDefs.h:58 > -#if HAVE(GLX) > +typedef uint32_t PlatformBufferHandle; > + > +#if USE(GLX) > typedef GLXContext PlatformContext; > typedef Display* PlatformDisplay; > typedef GLXFBConfig PlatformSurfaceConfig; > -typedef GLXDrawable PlatformSurface; > +typedef GLXDrawable PlatformDrawable; > #else > typedef void* PlatformContext; > typedef void* PlatformDisplay; > typedef void* PlatformSurfaceConfig; > -typedef void* PlatformSurface; > +typedef void* PlatformDrawable; why is this change not explained?
Created attachment 180388 [details] patch
Comment on attachment 180388 [details] patch K, Misunderstood the first comment. Will update a new patch
Created attachment 180390 [details] patch
(In reply to comment #5) > (From update of attachment 180231 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180231&action=review > HAVE, USE! done > > Source/WebCore/platform/graphics/opengl/GLDefs.h:58 > > why is this change not explained? Added comments in changelog file
Comment on attachment 180390 [details] patch in the future things like this should be two patches.
Comment on attachment 180390 [details] patch Rejecting attachment 180390 [details] from commit-queue. New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Full output: http://queues.webkit.org/results/15452137
Comment on attachment 180390 [details] patch Re-submitting patch to the queue because the failing inspector test is unrelated, probably flaky.
Comment on attachment 180390 [details] patch Clearing flags on attachment: 180390 Committed r138313: <http://trac.webkit.org/changeset/138313>
All reviewed patches have been landed. Closing bug.