Bug 116717

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

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.

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?

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

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.

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

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

Created attachment 203042 [details] Proposed patch Fixed style warnings Only include GtkWidgetBackingStoreX11.h when platform is GTK.

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

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.

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

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

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

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

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.

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

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.

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.

Created attachment 204005 [details] Reviewed patch with requested style fixes Addresses commented style issues.

Comment on attachment 204005 [details] Reviewed patch with requested style fixes Clearing flags on attachment: 204005 Committed r151398: <http://trac.webkit.org/changeset/151398>

All reviewed patches have been landed. Closing bug.