[Webkit-unassigned] [Bug 163333] Prefer eglGetPlatformDisplay to eglGetDisplay

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=163333

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #291358|                            |review-
              Flags|                            |

--- Comment #1 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 291358
  --> https://bugs.webkit.org/attachment.cgi?id=291358
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161013/f894b874/attachment.html>