Bug 149794 – GLContext should control ownership of context-related objects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

GLContext should control ownership of context-related objects

[Zan Dobersek]

Created attachment 262395 [details]
Patch

[Martin Robinson]

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.

[Zan Dobersek]

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.

[Martin Robinson]

(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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 262419 [details]
Patch

[Zan Dobersek]

Comment on attachment 262419 [details]
Patch

Clearing flags on attachment: 262419

Committed r190558: <http://trac.webkit.org/changeset/190558>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.