[webkit-reviews] review denied: [Bug 116717] [GTK] Do not use X11 WidgetBackingStore implementation in Wayland : [Attachment 203042] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Iago Toral
<itoral at igalia.com>'s request for review:
Bug 116717: [GTK] Do not use X11 WidgetBackingStore implementation  in Wayland
https://bugs.webkit.org/show_bug.cgi?id=116717

Attachment 203042: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=203042&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=203042&action=review


Nice work. I have a few small issues with the design, but the general approach
looks great.

> Source/WebCore/platform/cairo/WidgetBackingStore.cpp:44
> +namespace WebCore {
> +
> +PassOwnPtr<WidgetBackingStore> WidgetBackingStore::create(PlatformWidget
widget, const IntSize& size)
> +{
> +#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2)
> +    GdkDisplay* display =
gdk_display_manager_get_default_display(gdk_display_manager_get());
> +    if (GDK_IS_X11_DISPLAY(display))
> +	   return WebCore::GtkWidgetBackingStoreX11::create(widget, size);
> +#endif
> +    return WebCore::WidgetBackingStoreCairo::create(widget, size);
> +}

Instead of putting all this GTK+ specific code here, just move it to WebKit.
Have the GTK+ implementation do the check and have the EFL implementation just
instantiate a Cairo backing store. Since this code is mainly GTK+-specific it
probably should not go into the cairo directory.

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.h:26
> +class WidgetBackingStorePrivate;

You can now move the data that was once stored in WidgetBackingStorePrivate to
be a member of this concrete class and ditch the private class entirely.

> Source/WebCore/platform/gtk/GtkWidgetBackingStoreX11.cpp:20
> +#include "GtkWidgetBackingStoreX11.h"

Probably better to call this WidgetBackingStoreGtkX11. I know the filename if
funky (that's my fault, feel free to fix it or not), but let's keep the class
names a bit more consistent. :)