Bug 163722 - [GTK] Avoid including egl.h headers in internal headers
Summary: [GTK] Avoid including egl.h headers in internal headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2016-10-20 04:13 PDT by Carlos Garcia Campos
Modified: 2016-10-20 05:40 PDT (History)
2 users (show)

See Also:


Attachments
Patch (29.58 KB, patch)
2016-10-20 04:16 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-10-20 04:13:04 PDT
egl.h includes eglplatform.h that decides the native types for the platform at compile time. However, we support to build with X11 and Wayland at the same time and decide what to use at runtime. Currently GLContext.h includes eglplatform.h after wayland-egl.h if Wayland is enabled. That means that the wayland native types are used by default from all cpp files including GLContext.h. It currently works on X11 because we cast the value anyway and for example EGLNativeWindowType is a pointer in Wayland that can be casted to unsigned long in X11 to represent the X Window. This is very fragile in any case, we should avoid adding egl headers in our headers and only include it in cpp files. But we also need to ensure we don't use X11 and Wayland in the same cpp file.
Comment 1 Carlos Garcia Campos 2016-10-20 04:16:50 PDT
Created attachment 292166 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-20 04:18:28 PDT
Attachment 292166 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2016-10-20 05:29:24 PDT
Comment on attachment 292166 [details]
Patch

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

> Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:79
> +    return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(wlSurface), window));

Is std::make_unique<>() usable here?

> Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp:99
> +    return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(pixmap)));

Is std::make_unique<>() usable here?
Comment 4 Carlos Garcia Campos 2016-10-20 05:37:11 PDT
(In reply to comment #3)
> Comment on attachment 292166 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292166&action=review
> 
> > Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:79
> > +    return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(wlSurface), window));
> 
> Is std::make_unique<>() usable here?
> 
> > Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp:99
> > +    return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(pixmap)));
> 
> Is std::make_unique<>() usable here?

Nope, constructors are private to ensure only the create methods can create instances.
Comment 5 Carlos Garcia Campos 2016-10-20 05:40:48 PDT
Committed r207615: <http://trac.webkit.org/changeset/207615>