Bug 105872 – [EFL] [WebGL] Minor cleanup in Platform Context class.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kalyan]

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.

[Kalyan]

Created attachment 180959 [details]
patch

[Kalyan]

(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

[Kenneth Rohde Christiansen]

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

[Kalyan]

Created attachment 180966 [details]
patch

[Kalyan]

(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.

[Kenneth Rohde Christiansen]

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

[Kalyan]

Created attachment 180980 [details]
patchv3

[Kalyan]

(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

[Kenneth Rohde Christiansen]

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

[Kalyan]

Created attachment 180988 [details]
patchv4

[Kalyan]

(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.

[WebKit Review Bot]

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

[Sudarsana Nagineni (babu)]

Comment on attachment 180988 [details]
patchv4

Looks like inspector-protocol test is flaky and causing ews/cq failure.

[WebKit Review Bot]

Comment on attachment 180988 [details]
patchv4

Clearing flags on attachment: 180988

Committed r138620: <http://trac.webkit.org/changeset/138620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.