Bug 105872

Summary: [EFL] [WebGL] Minor cleanup in Platform Context class.
Product: WebKit Reporter: Kalyan <kalyan.kondapally>
Component: WebKit EFLAssignee: 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 Flags
patch
none
patch
none
patchv3
none
patchv4 none

Description Kalyan 2012-12-30 11:37:11 PST
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.
Comment 1 Kalyan 2012-12-30 13:00:51 PST
Created attachment 180959 [details]
patch
Comment 2 Kalyan 2012-12-30 13:06:17 PST
(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 3 Kenneth Rohde Christiansen 2012-12-30 14:08:16 PST
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
Comment 4 Kalyan 2012-12-31 00:05:12 PST
Created attachment 180966 [details]
patch
Comment 5 Kalyan 2012-12-31 00:07:36 PST
(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 6 Kenneth Rohde Christiansen 2012-12-31 02:18:04 PST
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
Comment 7 Kalyan 2012-12-31 12:38:18 PST
Created attachment 180980 [details]
patchv3
Comment 8 Kalyan 2012-12-31 12:39:31 PST
(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 9 Kenneth Rohde Christiansen 2012-12-31 13:14:15 PST
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
Comment 10 Kalyan 2013-01-01 04:22:19 PST
Created attachment 180988 [details]
patchv4
Comment 11 Kalyan 2013-01-01 04:24:05 PST
(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 12 WebKit Review Bot 2013-01-01 05:05:45 PST
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 13 Sudarsana Nagineni (babu) 2013-01-02 08:32:00 PST
Comment on attachment 180988 [details]
patchv4

Looks like inspector-protocol test is flaky and causing ews/cq failure.
Comment 14 WebKit Review Bot 2013-01-02 08:47:48 PST
Comment on attachment 180988 [details]
patchv4

Clearing flags on attachment: 180988

Committed r138620: <http://trac.webkit.org/changeset/138620>
Comment 15 WebKit Review Bot 2013-01-02 08:47:53 PST
All reviewed patches have been landed.  Closing bug.