[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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #252709|review?                     |review+
              Flags|                            |

--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 252709
  --> https://bugs.webkit.org/attachment.cgi?id=252709
Addressed review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=252709&action=review

> Source/WebCore/platform/graphics/glx/GLContextGLX.h:62
>      XID m_window;

If we initialized this here then all the constructors would be simpler.

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

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:199
> +    m_image.reset();

We often write this in the WebKit project as:

    m_image = nullptr;

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

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

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

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

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

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:54
> +        : m_resource(0)

Would be better to initialize to zero when m_resource is defined below.

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

WebKit project uses "unsigned long", not "long unsigned".

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

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

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

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

-- 
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/20150509/04135443/attachment-0001.html>