Bug 116717 – [GTK] Do not use X11 WidgetBackingStore implementation in Wayland

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Iago Toral]

When running on Wayland we cannot use the X11 implementation of the WidgetBackingStore. Although we do not have a Wayland specific implementation we do have a Cairo implementation that works in Wayland so we should at least use that for now.

I'll propose a patch to do this shortly.

[Iago Toral]

Created attachment 202804 [details]
Proposed patch

The patch does the following:

1) Refactor GtkWidgetBackingStore to be an abstract base class.
2) Make GtkWidgetBackingStoreX11 and WidgetBackingStoreCairo inherit from WidgetBackingStore.
3) When running on Wayland, use WidgetBackingStoreCairo instead of GtkWidgetBackingStoreX11

[Iago Toral]

Don't review this patch yet, the code in Source/WebKit2/UIProcess/cairo/BackingStoreCairo.cpp always uses the cairo implementation and this is probably not correct.

I was confused by the fact that this code works with two different platform widgets and the only widgetbackingstore implementation that supports this is the cairo one.

I guess we always need to use Cairo when using EFL, but in other case we should cairo or X11 depending on whether we are running or Wayland or not.

I'll check this and update the patch if necessary.

[Iago Toral]

Created attachment 202961 [details]
Proposed patch

This patch does the same as the previous one but handles the wk2 case properly. Some considerations:

1) I have concentrated the run-time decision about the backend to use (cairo or x11) in WidgetBackingStore::create.
2) WidgetBackingStore.cpp is part of libwebcoregtk , and hence needs to be built with GTK2 and GTK3 even if I understand there is no case in which the plugin process would use the WidgetBackingStore. The fallback case is to use the Cairo implementation.
3) I am not sure that WidgetBackingStore.{h,cpp} should live in WebCore/platform/cairo, would WebCore/platform be a better place?

[Iago Toral]

(In reply to comment #3)
> 1) I have concentrated the run-time decision about the backend to use (cairo or x11) in WidgetBackingStore::create.
> 2) WidgetBackingStore.cpp is part of libwebcoregtk , and hence needs to be built with GTK2 and GTK3 even if I understand there is no case in which the plugin process would use the WidgetBackingStore. The fallback case is to use the Cairo implementation.
> 3) I am not sure that WidgetBackingStore.{h,cpp} should live in WebCore/platform/cairo, would WebCore/platform be a better place?

4) I also patched the cmake files for  GTK and EFL, but I am not familiar with cmake myself so let me know if something else is needed here.

[WebKit Commit Bot]

Attachment 202961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/PlatformGTK.cmake', u'Source/WebCore/platform/cairo/WidgetBackingStore.cpp', u'Source/WebCore/platform/cairo/WidgetBackingStore.h', u'Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp', u'Source/WebCore/platform/cairo/WidgetBackingStoreCairo.h', u'Source/WebCore/platform/gtk/GtkWidgetBackingStoreX11.cpp', u'Source/WebCore/platform/gtk/GtkWidgetBackingStoreX11.h']" exit_code: 1
Source/WebCore/platform/cairo/WidgetBackingStore.cpp:20:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/platform/cairo/WidgetBackingStore.cpp:30:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 202961 [details]
Proposed patch

Attachment 202961 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/661034

[EFL EWS Bot]

Comment on attachment 202961 [details]
Proposed patch

Attachment 202961 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/754060

[Iago Toral]

Created attachment 203042 [details]
Proposed patch

Fixed style warnings
Only include GtkWidgetBackingStoreX11.h when platform is GTK.

[Martin Robinson]

Comment on attachment 203042 [details]
Proposed patch

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

[Iago Toral]

Created attachment 203142 [details]
Fixed patch

Changed patch according to review.
I kept the GtkWidgetBackingStoreX11.{cpp,h} file names in this patch but I can address that in a later patch.

[kov's GTK+ EWS bot]

Comment on attachment 203142 [details]
Fixed patch

Attachment 203142 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/659103

[Build Bot]

Comment on attachment 203142 [details]
Fixed patch

Attachment 203142 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/713213

New failing tests:
fast/dom/location-new-window-no-crash.html

[Build Bot]

Created attachment 203187 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[EFL EWS Bot]

Comment on attachment 203142 [details]
Fixed patch

Attachment 203142 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/656513

[Iago Toral]

Created attachment 203213 [details]
Fixed patch

Use <> notation for inclusion of WebCore files in WebKit2, should address build issue in the Efl port.
I could not reproduce build issue with the Gtk port though.

[kov's GTK+ EWS bot]

Comment on attachment 203213 [details]
Fixed patch

Attachment 203213 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/747203

[Iago Toral]

Created attachment 203335 [details]
Fixed patch

This should fix also the build for the gtk port, the problem was that definitions included through X11 headers clash with some other definitions included before. For some reason did does not happen for me, but I managed to reproduce the problem in another machine.

[Martin Robinson]

Comment on attachment 203335 [details]
Fixed patch

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

Looks good, but I have a few style nits.

> Source/WebCore/platform/gtk/GtkWidgetBackingStoreX11.cpp:47
> +    m_pixmap = XCreatePixmap(m_display,
> +        GDK_WINDOW_XID(gdk_screen_get_root_window(screen)),
> +        size.width(), size.height(),
> +        gdk_visual_get_depth(visual));

This statement would be short enough to put all the arguments on one line.

> Source/WebKit2/UIProcess/cairo/BackingStoreCairo.cpp:33
> +#if PLATFORM(GTK) && defined(GDK_WINDOWING_X11)
> +#include <WebCore/GtkWidgetBackingStoreX11.h>
> +#include <gdk/gdkx.h>
> +#endif

This block should go after the main one separated by a blank line.

> Source/WebKit2/UIProcess/cairo/BackingStoreCairo.cpp:50
> +static OwnPtr<WidgetBackingStore> createBackingStore(GtkWidget* widget, const IntSize& size)

Maybe this can be called createBackingStoreForGTK.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:80
> +#ifdef GDK_WINDOWING_X11
> +#define Font XFont
> +#define Cursor XCursor
> +#define Region XRegion
> +#include <gdk/gdkx.h>
> +#undef Font
> +#undef Cursor
> +#undef Region
> +#undef None
> +#undef Status
> +#endif

Again, this block should go after the main one and separated by a newline.

[Iago Toral]

Created attachment 204005 [details]
Reviewed patch with requested style fixes

Addresses commented style issues.

[WebKit Commit Bot]

Comment on attachment 204005 [details]
Reviewed patch with requested style fixes

Clearing flags on attachment: 204005

Committed r151398: <http://trac.webkit.org/changeset/151398>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.