[Webkit-unassigned] [Bug 144521] [X11] Add XUniquePtr and XUniqueResource to automatically free X resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144521

--- Comment #15 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #14)
> Comment on attachment 252709 [details]
> Addressed review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252709&action=review

Thanks!

> > Source/WebCore/platform/graphics/glx/GLContextGLX.h:62
> >      XID m_window;
> 
> If we initialized this here then all the constructors would be simpler.

Ok.

> > Source/WebCore/platform/graphics/glx/GLContextGLX.h:66
> >      cairo_device_t* m_cairoDevice;
> 
> If we initialized this here then all the constructors would be simpler.

Ok.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:199
> > +    m_image.reset();
> 
> We often write this in the WebKit project as:
> 
>     m_image = nullptr;

Sure. 

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:223
> > +    m_image.reset();
> 
> We often write this in the WebKit project as:
> 
>     m_image = nullptr;
> 
> > Source/WebCore/platform/graphics/x11/XUniquePtr.h:43
> > +template<typename T>
> > +struct XPtrDeleter {
> > +    void operator()(T* ptr) const { XFree(ptr); }
> > +};
> 
> 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?

It is not, I assumed XFree was null-safe like free. I'll fix it.

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

My original idea was to leave the macros public so that other X pointers like GLX could expand it just by using the macros, but in the end all code is here, so we can indeed get rid of the macros.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:44
> > +template<> void XUniqueResource<XResource::Colormap>::deleteXResource(long unsigned resource)
> 
> I don’t understand why this works in a cpp file. As I understand it,
> template specializations need to be in headers.

In this case, the template is defined as template <XResource T>, being XResource an enum class. I think this makes the compiler know the specializations that need to be defined, and fails with linking errors if any of them is missing.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:47
> > +        XFreeColormap(downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(), resource);
> 
> The repeated code
> downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native()
> should be a helper function, not written out each time.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:51
> > +template <> void XUniqueResource<XResource::Damage>::deleteXResource(long unsigned resource)
> 
> Would be nice to be consistent about whether there is a space after template.

Good catch, this is just wrong, we never leave a space after template.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:54
> > +        : m_resource(0)
> 
> Would be better to initialize to zero when m_resource is defined below.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:79
> > +    long unsigned get() const { return m_resource; }
> 
> WebKit project uses "unsigned long", not "long unsigned".

oops, I copy paste from X definition.

> > 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;
> > +    }
> 
> explicit operator bool is the modern way to do this, unless you need to be
> compatible with some broken compiler.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:98
> > +    XUniqueResource(const XUniqueResource&) = delete;
> > +    XUniqueResource& operator=(const XUniqueResource&) = delete;
> 
> These are unneeded. Defining move constructor and move assignment operator
> does this automatically.

I'll remove them.

> > 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
> 
> Seems pretty weak that these aren’t checked at all. You could easily use the
> wrong one.

I'm not sure I get what you want. These are just for convenience, to avoid having to write XUniqueResource<XResource::Foo> everywhere, and use XUniqueFoo instead.

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:205
> > +    // Explicit reset these because we need to ensure it happens in this order.
> 
> Typo: Explicitly is missing the "ly".

Oops.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150510/0e997984/attachment.html>