GLContext should control ownership of context-related objects
Created attachment 262395 [details] Patch
Comment on attachment 262395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262395&action=review > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:155 > + struct wl_surface* surface = wl_compositor_create_surface(m_compositor); > + EGLNativeWindowType nativeWindow = wl_egl_window_create(surface, 1, 1); > + auto contextData = std::make_unique<WaylandOffscreenContextData>(surface, nativeWindow); > + It's a bit strange how the window and surface are created outside of the Data object and then destroyed within it. It might not be obvious when using this data structure that it will eventually destroy what you pass to it. Why not: 1. Have the WaylandOffscreenContextData itself create the Window and surface. 2. Pass the WaylandOffscreenContextData to the constructor of GLContextEGL instead of setting it after creation. This will make the code a bit simpler and prevent forgetting to call GLContext::setContextData.
Comment on attachment 262395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262395&action=review >> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:155 >> + > > It's a bit strange how the window and surface are created outside of the Data object and then destroyed within it. It might not be obvious when using this data structure that it will eventually destroy what you pass to it. > > Why not: > > 1. Have the WaylandOffscreenContextData itself create the Window and surface. > 2. Pass the WaylandOffscreenContextData to the constructor of GLContextEGL instead of setting it after creation. This will make the code a bit simpler and prevent forgetting to call GLContext::setContextData. 1. That's possible, but would require passing the wl_compositor object to the constructor and adding a getter to the class to make the native window object accessible. 2. The parameter should be optional then, as not every GLContext creation procedure would require it. OK, noted.
(In reply to comment #3) > > 1. Have the WaylandOffscreenContextData itself create the Window and surface. > > 2. Pass the WaylandOffscreenContextData to the constructor of GLContextEGL instead of setting it after creation. This will make the code a bit simpler and prevent forgetting to call GLContext::setContextData. > > 1. That's possible, but would require passing the wl_compositor object to > the constructor and adding a getter to the class to make the native window > object accessible. Yeah, that seems like a decent approach. Alternatively, just make it a struct. > 2. The parameter should be optional then, as not every GLContext creation > procedure would require it. I agree that would be the best way to do it.
(In reply to comment #4) > (In reply to comment #3) > > > > 1. Have the WaylandOffscreenContextData itself create the Window and surface. > > > 2. Pass the WaylandOffscreenContextData to the constructor of GLContextEGL instead of setting it after creation. This will make the code a bit simpler and prevent forgetting to call GLContext::setContextData. > > > > 1. That's possible, but would require passing the wl_compositor object to > > the constructor and adding a getter to the class to make the native window > > object accessible. > > Yeah, that seems like a decent approach. Alternatively, just make it a > struct. > There's not much difference between classes and structs in C++, but the virtual destructor is required here, and that IMO fits more into a class than a struct (though it works in both). Publicly exposing the member objects isn't common, but wouldn't bother me that much. Furthermore, the class could be made local to this specific method, i.e. not declared in the WebCore namespace, keeping the implementation under control.
Created attachment 262419 [details] Patch
Comment on attachment 262419 [details] Patch Clearing flags on attachment: 262419 Committed r190558: <http://trac.webkit.org/changeset/190558>
All reviewed patches have been landed. Closing bug.