Bug 144521

Created attachment 252228 [details] Patch

Created attachment 252270 [details] Rebased patch Just rebased, it should apply now.

hmm, it seems there's a PlatformDisplay already defined when building with GRAPHICS_SURFACE enabled.

Created attachment 252479 [details] Rebased

Created attachment 252480 [details] Try to gix EFL build

Created attachment 252483 [details] Try to fix EFL build

Created attachment 252485 [details] Try to fix EFL build

Created attachment 252487 [details] Try to fix EFL build

Comment on attachment 252487 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=252487&action=review > Source/WebCore/PlatformEfl.cmake:200 > + platform/graphics/x11/XUnique.cpp Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp. > Source/WebCore/platform/graphics/x11/XUnique.h:124 > + long unsigned release() { long unsigned resource = m_resource; m_resource = 0; return resource; } Use std::exchange() here. (It's C++14, but there's an implementation in StdLibExtras.h.) > Source/WebCore/platform/graphics/x11/XUnique.h:126 > + void reset(long unsigned resource = 0) Instead of `resource.reset()`, I'd prefer resource = nullptr; If you consider it, it possible to implement by overloading the assignment operator: XUniqueResource& operator=(std::nullptr_t) { ... } > Source/WebCore/platform/graphics/x11/XUnique.h:164 > +typedef XUniqueResource<XResource::Colormap> XUniqueColormap; > +#if PLATFORM(GTK) > +typedef XUniqueResource<XResource::Damage> XUniqueDamage; > +#endif > +typedef XUniqueResource<XResource::Pixmap> XUniquePixmap; > +typedef XUniqueResource<XResource::Window> XUniqueWindow; > +#if USE(GLX) > +typedef XUniqueResource<XResource::GLXPbuffer> XUniqueGLXPbuffer; > +typedef XUniqueResource<XResource::GLXPixmap> XUniqueGLXPixmap; > +#endif > + > +// Give a name also to these XUniquePtr to avoid having to use the internal struct names (_XGC, __GLXcontextRec, ...). > +typedef XUniquePtr<_XGC> XUniqueGC; > +#if USE(GLX) > +typedef XUniquePtr<__GLXcontextRec> XUniqueGLXContext; > +#endif Use type aliases: using XUniqueWhatever = XUniqueHandler<Whatever>;

(In reply to comment #9) > Comment on attachment 252487 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252487&action=review Thanks for the review. > > Source/WebCore/PlatformEfl.cmake:200 > > + platform/graphics/x11/XUnique.cpp > > Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and > renaming XUnique.cpp to XUniqueResource.cpp. Why? I think it's quite simple and reduces the amount of header includes. > > Source/WebCore/platform/graphics/x11/XUnique.h:124 > > + long unsigned release() { long unsigned resource = m_resource; m_resource = 0; return resource; } > > Use std::exchange() here. (It's C++14, but there's an implementation in > StdLibExtras.h.) Cool. > > Source/WebCore/platform/graphics/x11/XUnique.h:126 > > + void reset(long unsigned resource = 0) > > Instead of `resource.reset()`, I'd prefer resource = nullptr; > If you consider it, it possible to implement by overloading the assignment > operator: > > XUniqueResource& operator=(std::nullptr_t) { ... } But this resource is not a pointer. I added reset for consistency with std::unique_ptr, and didn't add the assign operator for nullptr, because it doesn't store a pointer. > > Source/WebCore/platform/graphics/x11/XUnique.h:164 > > +typedef XUniqueResource<XResource::Colormap> XUniqueColormap; > > +#if PLATFORM(GTK) > > +typedef XUniqueResource<XResource::Damage> XUniqueDamage; > > +#endif > > +typedef XUniqueResource<XResource::Pixmap> XUniquePixmap; > > +typedef XUniqueResource<XResource::Window> XUniqueWindow; > > +#if USE(GLX) > > +typedef XUniqueResource<XResource::GLXPbuffer> XUniqueGLXPbuffer; > > +typedef XUniqueResource<XResource::GLXPixmap> XUniqueGLXPixmap; > > +#endif > > + > > +// Give a name also to these XUniquePtr to avoid having to use the internal struct names (_XGC, __GLXcontextRec, ...). > > +typedef XUniquePtr<_XGC> XUniqueGC; > > +#if USE(GLX) > > +typedef XUniquePtr<__GLXcontextRec> XUniqueGLXContext; > > +#endif > > Use type aliases: > using XUniqueWhatever = XUniqueHandler<Whatever>; Cool!

Comment on attachment 252487 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=252487&action=review >>> Source/WebCore/PlatformEfl.cmake:200 >>> + platform/graphics/x11/XUnique.cpp >> >> Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp. > > Why? I think it's quite simple and reduces the amount of header includes. Because over the lifetime of the WebKit project we’ve found that using a separate header for each class is a better pattern. Headers that have sets of classes might make sense when you are writing the code, but they can be frustrating later when reading and maintaining the code; a more mechanical rule is easier to understand and works better for someone who thinks slightly differently than the original author about categories.

(In reply to comment #11) > Comment on attachment 252487 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252487&action=review > > >>> Source/WebCore/PlatformEfl.cmake:200 > >>> + platform/graphics/x11/XUnique.cpp > >> > >> Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp. > > > > Why? I think it's quite simple and reduces the amount of header includes. > > Because over the lifetime of the WebKit project we’ve found that using a > separate header for each class is a better pattern. Headers that have sets > of classes might make sense when you are writing the code, but they can be > frustrating later when reading and maintaining the code; a more mechanical > rule is easier to understand and works better for someone who thinks > slightly differently than the original author about categories. Fair enough!

Created attachment 252709 [details] Addressed review comments

Comment on attachment 252709 [details] 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".

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

Created attachment 252809 [details] Patch for landing

Committed r184197: <http://trac.webkit.org/changeset/184197>