Bug 144517

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]