WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
149794
GLContext should control ownership of context-related objects
https://bugs.webkit.org/show_bug.cgi?id=149794
Summary
GLContext should control ownership of context-related objects
Zan Dobersek
Reported
2015-10-04 01:35:32 PDT
GLContext should control ownership of context-related objects
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2015-10-04 01:53:33 PDT
Created
attachment 262395
[details]
Patch
Martin Robinson
Comment 2
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.
Zan Dobersek
Comment 3
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.
Martin Robinson
Comment 4
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.
Zan Dobersek
Comment 5
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.
Zan Dobersek
Comment 6
2015-10-04 22:49:52 PDT
Created
attachment 262419
[details]
Patch
Zan Dobersek
Comment 7
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
>
Zan Dobersek
Comment 8
2015-10-05 00:14:35 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug