There are several places were we are handling the X display connection: - GLContext::sharedX11Display() creates a new connection. - X11Helper::nativeDisplay() creates a new connection. - BackingStoreBackendCairoX11 uses the GTK+ shared connection. - NetscapePlugin::x11HostDisplay() uses the GTK+/ecore shared connection - The rest of the GTK+ code uses the shared GTK+ connection And then we also have WaylandDisplay and the code to check if the current display is wayland or X11. We could unify all these to share the same connection to reduce the amount of ifdefs and ensure a single connection. That will also allow us to use "smart pointers" for the X resources that need a Display* to be freed.
Created attachment 252223 [details] Patch Zan, I haven't checked the Wayland part builds/works.
Attachment 252223 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/PlatformDisplay.h:42: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 252223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252223&action=review Excellent clean up. > Source/WebCore/platform/graphics/PlatformDisplay.h:65 > +#define SPECIALIZE_TYPE_TRAITS_PLATFORM_DISPLAY(ToClassName, DisplayType) \ > +SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::ToClassName) \ > + static bool isType(const WebCore::PlatformDisplay& display) { return display.type() == WebCore::PlatformDisplay::Type::DisplayType; } \ > +SPECIALIZE_TYPE_TRAITS_END() This is cool. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:242 > #if PLATFORM(X11) > - Display* display = GLContext::sharedX11Display(); > - m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(display)); > + m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native())); > #elif PLATFORM(WAYLAND) > - EGLDisplay display = WaylandDisplay::instance()->eglDisplay(); > - m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(display)); > + m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(downcast<PlatformDisplayWayland>(sharedDisplay).native())); > #endif These are #ifs but they really should both co-exist. It's unrelated to this patch though. > Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:317 > { > - // Display connection will only be broken at program shutdown. > - static DisplayConnection displayConnection; > - return displayConnection.display(); > + return downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(); Maybe these helpers can gradually move into PlatformDisplayX11. > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36 > +#include <wayland-client.h> > + > +#include <wayland-egl.h> Nit: Extra empty line here.
(In reply to comment #3) > Comment on attachment 252223 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252223&action=review > > Excellent clean up. Thanks. > > Source/WebCore/platform/graphics/PlatformDisplay.h:65 > > +#define SPECIALIZE_TYPE_TRAITS_PLATFORM_DISPLAY(ToClassName, DisplayType) \ > > +SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::ToClassName) \ > > + static bool isType(const WebCore::PlatformDisplay& display) { return display.type() == WebCore::PlatformDisplay::Type::DisplayType; } \ > > +SPECIALIZE_TYPE_TRAITS_END() > > This is cool. Yes :-) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:242 > > #if PLATFORM(X11) > > - Display* display = GLContext::sharedX11Display(); > > - m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(display)); > > + m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native())); > > #elif PLATFORM(WAYLAND) > > - EGLDisplay display = WaylandDisplay::instance()->eglDisplay(); > > - m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(display)); > > + m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(downcast<PlatformDisplayWayland>(sharedDisplay).native())); > > #endif > > These are #ifs but they really should both co-exist. It's unrelated to this > patch though. Right, I tried to not change any logic in this patch. But the PlatformDisplay class has been thought with the idea of supporting X11 and wayland support in the same binary, indeed. > > Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:317 > > { > > - // Display connection will only be broken at program shutdown. > > - static DisplayConnection displayConnection; > > - return displayConnection.display(); > > + return downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(); > > Maybe these helpers can gradually move into PlatformDisplayX11. Yes, probably, but it seems to me that some of them are very specific to surfaces implementation, we could make it even more generic, though. > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36 > > +#include <wayland-client.h> > > + > > +#include <wayland-egl.h> > > Nit: Extra empty line here. I just copied this file, but yes, I'll fix it anyway.
Comment on attachment 252223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252223&action=review > Source/WebCore/platform/graphics/PlatformDisplay.cpp:49 > +PlatformDisplay* PlatformDisplay::createPlatformDisplay() Any reason why this doesn't use std::make_unique<>() and return a std::unique_ptr<>? > Source/WebCore/platform/graphics/PlatformDisplay.cpp:77 > +std::unique_ptr<PlatformDisplay> PlatformDisplay::s_sharedDisplay; I'd prefer this moved off the class and into the PlatformDisplay::sharedDisplay() method. Perhaps it would be wise to also use std::call_once(), given that in the future this might be accessed from multiple threads.
(In reply to comment #5) > Comment on attachment 252223 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252223&action=review > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:49 > > +PlatformDisplay* PlatformDisplay::createPlatformDisplay() > > Any reason why this doesn't use std::make_unique<>() and return a > std::unique_ptr<>? Not really, I changed that several times, it was initially a reference until I realized it could return nullptr. > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:77 > > +std::unique_ptr<PlatformDisplay> PlatformDisplay::s_sharedDisplay; > > I'd prefer this moved off the class and into the > PlatformDisplay::sharedDisplay() method. > Perhaps it would be wise to also use std::call_once(), given that in the > future this might be accessed from multiple threads. Sure, very good points, I'll update the patch. Thanks!
Created attachment 252265 [details] Patch for landing Addressed review comments
Attachment 252265 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/PlatformDisplay.h:42: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/graphics/PlatformDisplay.cpp:82: Missing space before { [whitespace/braces] [5] ERROR: Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r183731: <http://trac.webkit.org/changeset/183731>
(In reply to comment #9) > Committed r183731: <http://trac.webkit.org/changeset/183731> It broke the WinCairo build, cc-ing the port maintainers. ..\platform\graphics\egl\GLContextEGL.cpp(69): error C2653: 'PlatformDisplay' : is not a class or namespace name [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] ..\platform\graphics\egl\GLContextEGL.cpp(69): error C3536: 'sharedDisplay': cannot be used before it is initialized [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] ..\platform\graphics\egl\GLContextEGL.cpp(69): error C2064: term does not evaluate to a function taking 0 arguments [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]