Bug 107982 – Use USE(GLX) instead of HAVE(GLX) to be semantically right.

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Hugo Parente Lima]

In some system HAVE(GLX) could be true but it doesn't want to use it, e.g. WebKit using EGL on a X-less raspbery Pi setup.

[Hugo Parente Lima]

Created attachment 184803 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184803 [details]
Patch

Attachment 184803 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16097935

[Early Warning System Bot]

Comment on attachment 184803 [details]
Patch

Attachment 184803 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16122494

[Hugo Parente Lima]

Created attachment 184809 [details]
Patch

[Kalyan]

Comment on attachment 184809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184809&action=review

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:39
> +#if USE(GLX)

This is needed for EFL port for now. You could change it to #if HAVE(GLX) AND PLATFORM(EFL). Ideally we could get rid of this completely as the right includes get automatically added in GLDefs.h

[Hugo Parente Lima]

(In reply to comment #5)
> (From update of attachment 184809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184809&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:39
> > +#if USE(GLX)
> 
> This is needed for EFL port for now. You could change it to #if HAVE(GLX) AND PLATFORM(EFL). Ideally we could get rid of this completely as the right includes get automatically added in GLDefs.h

Using USE(GLX) also works with EFL, but if the build was a coincidence of parameters used on EWS bot I can use the same hack Efl already have on line 58:

USE(GLX) || PLATFORM(EFL)

Note that the hack is already using USE instead of HAVE.

[Kalyan]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 184809 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184809&action=review
> > 
> > > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:39
> > > +#if USE(GLX)
> > 

This is k if one configures to use GLX(well in that case you don't even need this), but when one chooses to use EGL that's when the problem comes. CurrentContextWrapper is used mostly on UI process side where evas is not configured to support EGL yet. 

USE(GLX) || PLATFORM(EFL) ->It could work in this manner also but this hack should not be needed outside EFL. Once we fix it in our side, we dont have to use HAVE at all but only either USE(GLX) or USE(EGL). Than this include will be cleaned up from here.

[Kalyan]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 184809 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=184809&action=review
> > > 
> > > > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:39
> > > > +#if USE(GLX)
> > > 
> 
> This is k if one configures to use GLX(well in that case you don't even need this), but when one chooses to use EGL that's when the problem comes. CurrentContextWrapper is used mostly on UI process side where evas is not configured to support EGL yet. 

Evas is the library which is used to provide us with a GL Context and Surface on UI process side for doing the accelerated composition.

[Kalyan]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 184809 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184809&action=review
> Using USE(GLX) also works with EFL, but if the build was a coincidence of parameters used on EWS bot I can use the same hack Efl already have on line 58:

Note that EWS bot uses GLX by default and EGL support needs to be explicitly enabled during configuration. This is not enabled by default as the implementation is not complete yet.

[Laszlo Gombos]

(In reply to comment #0)
> In some system HAVE(GLX) could be true but it doesn't want to use it, e.g. WebKit using EGL on a X-less raspbery Pi setup.

The motivation of being able to disable this flag is a valid one, but I am not sure if this warrant changing the name. This seems to be a gray area.

A similar flag is HAVE(ACCESSIBILITY) that can be disabled build time (e.g. for EFL).

The definition of HAVE():
Describe properties of the target environment, a specific system features (headers, functions or similar) that are present or not.

The definition of USE():
Policy decision macros - defines policy choice for a particular port. Use a particular third-party library or optional OS service.

[Kalyan]

(In reply to comment #10)
> (In reply to comment #0)

USE(GLX) || PLATFORM(EFL) ->It could work in this manner also but this hack 

k, this would also work.

> The definition of USE():
> Policy decision macros - defines policy choice for a particular port. Use a particular third-party library or optional OS service.

Ideally, correct thing is USE as it is more like a policy choice to either use EGL or GLX (i.e Linux which has support for both).

W.R.T changes in GLPlatformSurface:
IMHO USE is the right one as we really want to use either EGL or GLX as defined during configiration.

GLPLatformContext and GraphicsSurfaceGLX are a bit of hack here(existing code and not this patch) to get our port working with EGL support for now. Ideally, we don't need to include any GLX headers in GLPlatformContext. Both of these are needed till we are able to configure EVAS with EGL support. 
so, something like #if USE(GLX) || PLATFORM(EFL) sound k ??

[Kalyan]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #0)

sorry, GLPLatformContext and GraphicsSurfaceGLX  should be more like GLPLatformContext and GraphicsSurfaceToken.

[Kalyan]

(In reply to comment #12)
> sorry, GLPLatformContext and GraphicsSurfaceGLX  should be more like GLPLatformContext and GraphicsSurfaceToken.

As we are on this topic anyway, so to clean up things bit more:

W.r.t GraphicsSurfaceToken:

It is completely wrong to base the check on GL(GLX or EGL etc). As with EGL we still want to use X Drawables (i.e Window, pixmap etc) on X, while in some other cases we don't want to use X Drawables (like the case here). 
Shouldn't it rather be something like #if platform(X) ??

[Hugo Parente Lima]

In other words, better to wait, I'll abandon this change.

[Laszlo Gombos]

(In reply to comment #14)
> In other words, better to wait, I'll abandon this change.

Hugo, is this patch still up for review ? If not maybe you should clear the r? . Thanks !

[Hugo Parente Lima]

(In reply to comment #15)
> (In reply to comment #14)
> > In other words, better to wait, I'll abandon this change.
> 
> Hugo, is this patch still up for review ? If not maybe you should clear the r? . Thanks !

Done, sorry.