Bug 163333 – Prefer eglGetPlatformDisplay to eglGetDisplay

• Component: WebKitGTK
• Status: RESOLVED FIXED

Created attachment 291358 [details]
eglGetPlatformDisplay.patch

eglGetDisplay forces the implementation to guess what kind of void* it's been handed. Different implementations do different things, in particular glvnd and Mesa behave differently.

Fortunately there exists API to tell EGL what kind of display it is, so let's use it. Patch is against webkitgtk since that's where I hit this first. Probably this should be made a single utility function somewhere instead, but I don't know the webkit source well enough to know where to put it.

[Michael Catanzaro]

Comment on attachment 291358 [details]
eglGetPlatformDisplay.patch

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

Hi, thanks for the patch. A couple of comments on submitting patches to WebKit in general:

 * Be sure to select the WebKit Gtk Bugzilla component, or us Linux folks may not notice your patch for a very long time. I found this by luck, because I happened to check the Fedora git repo to see if Tom had packaged 2.14.1 yet. :)
 * Also be sure to set the r? and cq? Bugzilla flags for patches if you want them to be reviewed or committed.
 * The patch requires a changelog entry ('Tools/Scripts/PrepareChangeLog -b 291358').
 * The patch must be prepared against a git checkout rather than a release tarball.
 * Easiest way to submit the patch is to use 'Tools/Scripts/webkit-patch upload --request-commit', it will pick the bug number automatically from your changelog entry

'webkit-patch upload' would have warned you about dozens of code style errors, including:

 * The pointer star always goes on the type, not the variable name. "const char* client_extensions" rather than "const char *client_extensions"
 * No space before opening parentheses in function calls. "eglQueryString(" not "eglQueryString ("
 * Use nullptr rather than NULL
 * Use C++ casts, never C style casts. Prefer static_cast and use reinterpret_cast when that fails.
 * Always indent four spaces, no tabs.

As for how to deduplicate the code, I think the best option would be to use a protected static function in PlatformDisplay. The child class would pass either EGL_PLATFORM_WAYLAND_KHR or EGL_PLATFORM_X11_KHR and get back a value that can be assigned to m_eglDisplay.

Lastly, we have another open bug #161958 about how strstr is not the correct way to check for EGL extensions; seems like now would be a good time to tighten that up.

> webkitgtk-2.14.0/Source/WebCore/platform/graphics/egl/GLContextEGL.h:27
> +#include <EGL/eglext.h>

Is this an accident? You don't change anything else in this file, so I presume this include was meant to be added to PlatformDisplayWayland.cpp instead. It's important to minimize the number of include statements in header files since they can dramatically increase build time.

[Michael Catanzaro]

(In reply to comment #1)
>  * The patch requires a changelog entry ('Tools/Scripts/PrepareChangeLog -b
> 291358').

Whoops, it's late, that's supposed to be the bug number (163333) not the attachment number.

[Carlos Garcia Campos]

Created attachment 292171 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292171 [details]
Patch for landing

Clearing flags on attachment: 292171

Committed r207616: <http://trac.webkit.org/changeset/207616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

Committed r207619: <http://trac.webkit.org/changeset/207619>

[Csaba Osztrogonác]

(In reply to comment #4)
> Comment on attachment 292171 [details]
> Patch for landing
> 
> Clearing flags on attachment: 292171
> 
> Committed r207616: <http://trac.webkit.org/changeset/207616>

It broke the GTK build, see build.webkit.org for details.
It would be great to have same config on EWS and buildbots.