Bug 149794 - GLContext should control ownership of context-related objects
Summary: GLContext should control ownership of context-related objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-04 01:35 PDT by Zan Dobersek
Modified: 2015-10-05 00:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.81 KB, patch)
2015-10-04 01:53 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (6.91 KB, patch)
2015-10-04 22:49 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2015-10-04 01:35:32 PDT
GLContext should control ownership of context-related objects
Comment 1 Zan Dobersek 2015-10-04 01:53:33 PDT
Created attachment 262395 [details]
Patch
Comment 2 Martin Robinson 2015-10-04 08:19:28 PDT
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 3 Zan Dobersek 2015-10-04 08:30:25 PDT
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.
Comment 4 Martin Robinson 2015-10-04 08:37:23 PDT
(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.
Comment 5 Zan Dobersek 2015-10-04 08:49:37 PDT
(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.
Comment 6 Zan Dobersek 2015-10-04 22:49:52 PDT
Created attachment 262419 [details]
Patch
Comment 7 Zan Dobersek 2015-10-05 00:14:18 PDT
Comment on attachment 262419 [details]
Patch

Clearing flags on attachment: 262419

Committed r190558: <http://trac.webkit.org/changeset/190558>
Comment 8 Zan Dobersek 2015-10-05 00:14:35 PDT
All reviewed patches have been landed.  Closing bug.