currently, we check for any supported extensions and use them as found. The logic to check the support is scattered a bit in different classes i.e PlatformContext, eglcontext etc. It would be nice to have this in one place i.e in PlatformContext. Some of the functions in eglconfighelper should be declared with const keyword as they are meant to be read only.
Created attachment 180959 [details] patch
(In reply to comment #0) > currently, we check for any supported extensions and use them as found. The > Some of the functions in eglconfighelper should be declared with const keyword as they are meant to be read only. Will do confighelper changes in another changeset
Comment on attachment 180959 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=180959&action=review > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:105 > + if (supportedExtensions.contains(name)) > + return true; so what is one extension is the subname of another one? dont you need to look for spaces? This doesnt seem so robust > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:130 > +} > +#elif USE(GLX) > +bool GLPlatformContext::supportsGLXExtension(Display* display, const String& name) I think #endif is nicer here as they are differently named methods > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:48 > + if (GLPlatformContext::supportsGLXExtension(display, "GLX_ARB_create_context_robustness ")) I dont like that you have to add space here
Created attachment 180966 [details] patch
(In reply to comment #3) > (From update of attachment 180959 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180959&action=review > > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:48 > > + if (GLPlatformContext::supportsGLXExtension(display, "GLX_ARB_create_context_robustness ")) > > I dont like that you have to add space here k, I changed the logic so that we split the extension string and store the individual extension names. This should address all the issues now.
Comment on attachment 180966 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=180966&action=review > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:94 > +void GLPlatformContext::splitAndStoreExtensionNames(HashSet<String>& splitExtNames, const String& extensionsString) Why is this not a free function? > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:98 > + uint totalSize = extNames.size(); we normally just use "unsigned" size/length would be a better name
Created attachment 180980 [details] patchv3
(In reply to comment #6) > (From update of attachment 180966 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180966&action=review > > > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:94 > > +void GLPlatformContext::splitAndStoreExtensionNames(HashSet<String>& splitExtNames, const String& extensionsString) > > Why is this not a free function? made the relevant changes now. Changed the other private static functions too. > > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:98 > > + uint totalSize = extNames.size(); > > we normally just use "unsigned" > > size/length would be a better name done
Comment on attachment 180980 [details] patchv3 View in context: https://bugs.webkit.org/attachment.cgi?id=180980&action=review > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:70 > +void splitAndStoreExtensionNames(HashSet<String>& splitExtNames, const String& extensionsString) static! works on some platforms without > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:127 > + splitAndStoreExtensionNames(supportedExtensions, extensionsString); extractExtensionNames ? > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:146 > + retrievedExtensions = true; !supportedExtensions.length() ? > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:148 > + String extensionsString = reinterpret_cast<const char*>(eglQueryString(display, EGL_EXTENSIONS)); > + splitAndStoreExtensionNames(supportedExtensions, extensionsString); String adds no info. String extensions should be sufficient. Maybe static HastSet<String> extensions = parseExtensions(rawExtensions); > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:207 > + initializeResetStatusExtension(); resolveResetStatusExtension() sounds better. It is not really initializing
Created attachment 180988 [details] patchv4
(In reply to comment #9) > (From update of attachment 180980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180980&action=review > > > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:70 > > +void splitAndStoreExtensionNames(HashSet<String>& splitExtNames, const String& extensionsString) > > > + splitAndStoreExtensionNames(supportedExtensions, extensionsString); > > String adds no info. String extensions should be sufficient. done. > Maybe > > static HastSet<String> extensions = parseExtensions(rawExtensions); > looks k for me. changes done.
Comment on attachment 180988 [details] patchv4 Attachment 180988 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15625213 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 180988 [details] patchv4 Looks like inspector-protocol test is flaky and causing ews/cq failure.
Comment on attachment 180988 [details] patchv4 Clearing flags on attachment: 180988 Committed r138620: <http://trac.webkit.org/changeset/138620>
All reviewed patches have been landed. Closing bug.