Bug 136831

Created attachment 238138 [details] Patch

Attachment 238138 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 238138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238138&action=review > Source/WebCore/platform/graphics/GLContext.cpp:75 > +#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2) > + if (!WaylandDisplay::instance()) > + return nullptr; > + DEPRECATED_DEFINE_STATIC_LOCAL(OwnPtr<GLContext>, sharing, (WaylandDisplay::instance()->createSharingGLContext())); > +#else > DEPRECATED_DEFINE_STATIC_LOCAL(OwnPtr<GLContext>, sharing, (createOffscreenContext())); > +#endif > return sharing.get(); Can't you move this into createOffscreenContext? This seems like the wrong place, because it requires duplicating some things. > Source/WebCore/platform/graphics/GLContext.cpp:82 > + if (WaylandDisplay::instance()) > + return WaylandDisplay::instance()->nativeDisplay(); Why not just use this directly instead of creating and intermediary in another class? Or create a static WaylandDisplay::nativeDisplay()? > Source/WebCore/platform/graphics/GLContext.h:77 > +#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2) > + static struct wl_display* sharedWaylandDisplay(); > +#endif > + Why is this a static method? Is it used outside GLContext?

Comment on attachment 238138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238138&action=review >> Source/WebCore/platform/graphics/GLContext.cpp:75 >> return sharing.get(); > > Can't you move this into createOffscreenContext? This seems like the wrong place, because it requires duplicating some things. It doesn't really fit into createOffscreenContext(). I'll modify this method to remove DEPRECATED_DEFINE_STATIC_LOCAL() but still use WaylandDisplay::createSharingGLContext() when actually running under Wayland, and fall back to createOffscreenContext() otherwise. >> Source/WebCore/platform/graphics/GLContext.cpp:82 >> + return WaylandDisplay::instance()->nativeDisplay(); > > Why not just use this directly instead of creating and intermediary in another class? Or create a static WaylandDisplay::nativeDisplay()? As it stands this is only called in GLContextEGL, so I'll change the code there to call ::nativeDisplay() directly.

Created attachment 238163 [details] Patch

Attachment 238163 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 238165 [details] Patch

Attachment 238165 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 238199 [details] Patch

Attachment 238199 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 238249 [details] Patch

Attachment 238249 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 238249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238249&action=review > Source/WebCore/platform/graphics/GLContext.cpp:69 > + static OwnPtr<GLContext> context; Could this be a unique_ptr?

Comment on attachment 238249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238249&action=review >> Source/WebCore/platform/graphics/GLContext.cpp:69 >> + static OwnPtr<GLContext> context; > > Could this be a unique_ptr? The current DEPRECATED_DEFINE_STATIC_LOCAL() code switched to using std::unique_ptr<>, so the new patch will use that as well.

Created attachment 249057 [details] Patch

Attachment 249057 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 249057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249057&action=review > Source/WebCore/platform/graphics/GLContext.cpp:79 > + return context.get(); You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL Also I guess you could a static LazyNeverDestroyed + std::call_once() ?

Comment on attachment 249057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249057&action=review >> Source/WebCore/platform/graphics/GLContext.cpp:79 >> + return context.get(); > > You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL > > Also I guess you could a static LazyNeverDestroyed + std::call_once() ? Oops. std::call_once() isn't ideal here since this isn't cross-thread code (yet?), and you have to use a static std::once_flag (and the accompanying guard variable), so that's some additional overhead.

(In reply to comment #18) > Comment on attachment 249057 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249057&action=review > > >> Source/WebCore/platform/graphics/GLContext.cpp:79 > >> + return context.get(); > > > > You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL > > > > Also I guess you could a static LazyNeverDestroyed + std::call_once() ? > > Oops. > > std::call_once() isn't ideal here since this isn't cross-thread code (yet?), > and you have to use a static std::once_flag (and the accompanying guard > variable), so that's some additional overhead. Yeah well you can use the null check anyway, but NeverDestroyed is the natural replacement for DEPRECATED_DEFINE_STATIC_LOCAL.

Comment on attachment 249057 [details] Patch Attachment 249057 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/6030012943695872 Number of test failures exceeded the failure limit.

Created attachment 249128 [details] Archive of layout-test-results from ews203 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews203 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

Comment on attachment 249057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249057&action=review >>>> Source/WebCore/platform/graphics/GLContext.cpp:79 >>>> + return context.get(); >>> >>> You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL >>> >>> Also I guess you could a static LazyNeverDestroyed + std::call_once() ? >> >> Oops. >> >> std::call_once() isn't ideal here since this isn't cross-thread code (yet?), and you have to use a static std::once_flag (and the accompanying guard variable), so that's some additional overhead. > > Yeah well you can use the null check anyway, but NeverDestroyed is the natural replacement for DEPRECATED_DEFINE_STATIC_LOCAL. I'll very likely go with std::call_once(), because paranoia. NeverDestroyed should probably be used as well.

Created attachment 250697 [details] Patch

Attachment 250697 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 250697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250697&action=review Looks good to me, apart from one small question: > Source/WebCore/platform/graphics/GLContext.cpp:31 > +#include <wtf/NeverDestroyed.h> > #include <wtf/ThreadSpecific.h> These should move above #if USE(EGL) > Source/WebCore/platform/graphics/GLContext.cpp:-153 > -#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2) && defined(GDK_WINDOWING_WAYLAND) && USE(EGL) > - GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get()); > - > - if (GDK_IS_WAYLAND_DISPLAY(display)) { > - if (auto eglContext = GLContextEGL::createContext(windowHandle, sharingContext)) > - return WTF::move(eglContext); > - return nullptr; > - } > -#endif This breaks things doesn't it? It looks like GLX contexts will be preferred even when a Wayland display is active...

Comment on attachment 250697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250697&action=review > Source/WebCore/platform/graphics/GLContext.h:33 > +#elif PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2) > +#include <wayland-egl.h> > +#include <EGL/eglplatform.h> > +typedef EGLNativeWindowType GLNativeWindowType; This 4 lines on GlContext.h were just committed on http://trac.webkit.org/changeset/185198 I guess you need to rebase the patch.

Let's track this in bug #115803 instead. *** This bug has been marked as a duplicate of bug 115803 ***