Bug 105431 – [EFL] [WebGL] [WK2] Replace HAVE(GLX) checks with USE(GLX)

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

[Kalyan]

In our side, we check if GLX is supported or not. We should change this check to USE, to be able to configure  using EGL or GLX

[Kalyan]

Created attachment 180208 [details]
patch

[Laszlo Gombos]

Comment on attachment 180208 [details]
patch

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

I think renaming from HAVE() to USE() make sense. I only reviewed the EFL-only parts (OptionsEfl.cmake) to make the next round of review easier.

r- to fix the EFL CMake file and make it consistent with the intention of the patch.

> Source/cmake/OptionsEfl.cmake:170
>          add_definitions(-DHAVE_GLX)

I think this can be removed now.

> Source/cmake/OptionsEfl.cmake:171
> +        set(WTF_USE_GLX 1)

This is not needed - WTF_USE_GLX variable is not used anywhere in the make system so no need to set it.

> Source/cmake/OptionsEfl.cmake:172
> +        add_definitions(-DWTF_USE_GLX=1)

This is correct, this is the only definition that actually needed.

> Source/cmake/OptionsEfl.cmake:174
> +        set(ENABLE_GLX 1)
> +        add_definitions(-DENABLE_GLX=1)

Not needed, not used anywhere.

[Kalyan]

Created attachment 180231 [details]
patchv2

review fixes

[Kalyan]

(In reply to comment #2)
> (From update of attachment 180208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180208&action=review
> 
> > Source/cmake/OptionsEfl.cmake:174
> > +        set(ENABLE_GLX 1)
> > +        add_definitions(-DENABLE_GLX=1)
> 
> Not needed, not used anywhere.

Done.

[Kenneth Rohde Christiansen]

Comment on attachment 180231 [details]
patchv2

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

This looks like something which should be two patches

> ChangeLog:8
> +        This patch changes the check Have(GLX) to Use(GLX).

HAVE, USE!

> Source/WebCore/platform/graphics/opengl/GLDefs.h:58
> -#if HAVE(GLX)
> +typedef uint32_t PlatformBufferHandle;
> +
> +#if USE(GLX)
>  typedef GLXContext PlatformContext;
>  typedef Display* PlatformDisplay;
>  typedef GLXFBConfig PlatformSurfaceConfig;
> -typedef GLXDrawable PlatformSurface;
> +typedef GLXDrawable PlatformDrawable;
>  #else
>  typedef void* PlatformContext;
>  typedef void* PlatformDisplay;
>  typedef void* PlatformSurfaceConfig;
> -typedef void* PlatformSurface;
> +typedef void* PlatformDrawable;

why is this change not explained?

[Kalyan]

Created attachment 180388 [details]
patch

[Kalyan]

Comment on attachment 180388 [details]
patch

K, Misunderstood the first comment. Will update a new patch

[Kalyan]

Created attachment 180390 [details]
patch

[Kalyan]

(In reply to comment #5)
> (From update of attachment 180231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180231&action=review

> HAVE, USE!

done

> > Source/WebCore/platform/graphics/opengl/GLDefs.h:58
> 
> why is this change not explained?

Added comments in changelog file

[Kenneth Rohde Christiansen]

Comment on attachment 180390 [details]
patch

in the future things like this should be two patches.

[WebKit Review Bot]

Comment on attachment 180390 [details]
patch

Rejecting attachment 180390 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/15452137

[Viatcheslav Ostapenko]

Comment on attachment 180390 [details]
patch

Re-submitting patch to the queue because the failing inspector test is unrelated, probably flaky.

[WebKit Review Bot]

Comment on attachment 180390 [details]
patch

Clearing flags on attachment: 180390

Committed r138313: <http://trac.webkit.org/changeset/138313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.