Summary: | [EFL] [WebGL] Minor cleanup in Platform Context class. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kalyan <kalyan.kondapally> | ||||||||||
Component: | WebKit EFL | Assignee: | Kalyan <kalyan.kondapally> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, dino, kenneth, lucas.de.marchi, noam, webkit.review.bot, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 105659 | ||||||||||||
Attachments: |
|
Description
Kalyan
2012-12-30 11:37:11 PST
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. |