<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [X11] Add XUniquePtr and XUniqueResource to automatically free X resources"
href="https://bugs.webkit.org/show_bug.cgi?id=144521">bug 144521</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #252709 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [X11] Add XUniquePtr and XUniqueResource to automatically free X resources"
href="https://bugs.webkit.org/show_bug.cgi?id=144521#c14">Comment # 14</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [X11] Add XUniquePtr and XUniqueResource to automatically free X resources"
href="https://bugs.webkit.org/show_bug.cgi?id=144521">bug 144521</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=252709&action=diff" name="attach_252709" title="Addressed review comments">attachment 252709</a> <a href="attachment.cgi?id=252709&action=edit" title="Addressed review comments">[details]</a></span>
Addressed review comments
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=252709&action=review">https://bugs.webkit.org/attachment.cgi?id=252709&action=review</a>
<span class="quote">> Source/WebCore/platform/graphics/glx/GLContextGLX.h:62
> XID m_window;</span >
If we initialized this here then all the constructors would be simpler.
<span class="quote">> Source/WebCore/platform/graphics/glx/GLContextGLX.h:66
> cairo_device_t* m_cairoDevice;</span >
If we initialized this here then all the constructors would be simpler.
<span class="quote">> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:199
> + m_image.reset();</span >
We often write this in the WebKit project as:
m_image = nullptr;
<span class="quote">> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:223
> + m_image.reset();</span >
We often write this in the WebKit project as:
m_image = nullptr;
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniquePtr.h:43
> +template<typename T>
> +struct XPtrDeleter {
> + void operator()(T* ptr) const { XFree(ptr); }
> +};</span >
The version of this I wrote had a null check, and the documentation for XFree makes it clear it can’t be called with a null. Why is it OK to omit a null check here?
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniquePtr.h:74
> +DEFINE_XPTR_DELETER(XImage, XDestroyImage)
> +DEFINE_XPTR_DELETER_WITH_DISPLAY(_XGC, XFreeGC)
> +
> +#if USE(GLX)
> +DEFINE_XPTR_DELETER_WITH_DISPLAY(__GLXcontextRec, glXDestroyContext)
> +#endif</span >
I don’t think it’s a good idea to use macros for these. One is only used once and the other is only used twice. Writing that second template specialization twice seems like it would be fine.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:44
> +template<> void XUniqueResource<XResource::Colormap>::deleteXResource(long unsigned resource)</span >
I don’t understand why this works in a cpp file. As I understand it, template specializations need to be in headers.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:47
> + XFreeColormap(downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(), resource);</span >
The repeated code downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native() should be a helper function, not written out each time.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:51
> +template <> void XUniqueResource<XResource::Damage>::deleteXResource(long unsigned resource)</span >
Would be nice to be consistent about whether there is a space after template.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.h:54
> + : m_resource(0)</span >
Would be better to initialize to zero when m_resource is defined below.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.h:79
> + long unsigned get() const { return m_resource; }</span >
WebKit project uses "unsigned long", not "long unsigned".
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.h:95
> + // This conversion operator allows implicit conversion to bool but not to other integer types.
> + typedef void* (XUniqueResource::*UnspecifiedBoolType);
> + operator UnspecifiedBoolType*() const
> + {
> + return m_resource ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> + }</span >
explicit operator bool is the modern way to do this, unless you need to be compatible with some broken compiler.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.h:98
> + XUniqueResource(const XUniqueResource&) = delete;
> + XUniqueResource& operator=(const XUniqueResource&) = delete;</span >
These are unneeded. Defining move constructor and move assignment operator does this automatically.
<span class="quote">> Source/WebCore/platform/graphics/x11/XUniqueResource.h:114
> +using XUniqueColormap = XUniqueResource<XResource::Colormap>;
> +#if PLATFORM(GTK)
> +using XUniqueDamage = XUniqueResource<XResource::Damage>;
> +#endif
> +using XUniquePixmap = XUniqueResource<XResource::Pixmap>;
> +using XUniqueWindow = XUniqueResource<XResource::Window>;
> +#if USE(GLX)
> +using XUniqueGLXPbuffer = XUniqueResource<XResource::GLXPbuffer>;
> +using XUniqueGLXPixmap = XUniqueResource<XResource::GLXPixmap>;
> +#endif</span >
Seems pretty weak that these aren’t checked at all. You could easily use the wrong one.
<span class="quote">> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:205
> + // Explicit reset these because we need to ensure it happens in this order.</span >
Typo: Explicitly is missing the "ly".</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>