I want to change WebCore/platform/gtk/GtkWidgetBackingStore.h to WebCore/platform/WidgetBackingStore.h to use WidgetBackingStore class in the EFL port. My ultimate goal is to share the WebKit2/UIProcess/gtk/BackingStoreGtk.cpp with EFL port by changing it to WebKit2/UIProcess/cairo/BackingStoreCairo.cpp. When I try to make WebKit2/UIProcess/efl/BackingStoreEfl.cpp, there are many duplicated codes with GTK port because both of them use the "cairo". So, I want to integrate cairo codes in the GTK and EFL port. related bug : https://bugs.webkit.org/show_bug.cgi?id=62444
Created attachment 97582 [details] Patch
Dear Martin, I've made patch for sharing GtkWidgetBackingStore with EFL. I think that you know about GtkWidgetBackingStore very well because you are the author of that class. Would you review this patch? I really need your advice :)
Comment on attachment 97582 [details] Patch Attachment 97582 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8880568
Created attachment 97594 [details] Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the EFL port. Add missing file - WebCore/platform/WidgetBackingStore.h
Comment on attachment 97594 [details] Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the EFL port. Attachment 97594 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8878709
Created attachment 97740 [details] Patch to fix efl error remove EFL related codes to fix efl build break.
Comment on attachment 97740 [details] Patch to fix efl error View in context: https://bugs.webkit.org/attachment.cgi?id=97740&action=review > Source/WebCore/platform/WidgetBackingStore.h:7 > +/* > + * Copyright (C) 2011, Igalia S.L. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2 of the License, or (at your option) any later version. This file probably belongs in the Cairo subdirectory since it heavily depends on Cairo. > Source/WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp:34 > -static PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(GtkWidget* widget, const IntSize& size) > +static PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(PlatformWidget widget, const IntSize& size) > { > return gdk_window_create_similar_surface(gtk_widget_get_window(widget), > CAIRO_CONTENT_COLOR_ALPHA, I'm not sure I understand this part of the change. Here we abstract away that the widget is a GTK+ widget and then immediately call gtk_widget_get_window on it.
Created attachment 97925 [details] Patch for comments
Thank you for your comments :) I've modified as follows: (In reply to comment #7) > (From update of attachment 97740 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97740&action=review > > > Source/WebCore/platform/WidgetBackingStore.h:7 > > +/* > > + * Copyright (C) 2011, Igalia S.L. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2 of the License, or (at your option) any later version. > > This file probably belongs in the Cairo subdirectory since it heavily depends on Cairo. > I added Source/WebCore/platform/cairo directory and moved WidgetBackingStore.h to there. > > Source/WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp:34 > > -static PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(GtkWidget* widget, const IntSize& size) > > +static PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(PlatformWidget widget, const IntSize& size) > > { > > return gdk_window_create_similar_surface(gtk_widget_get_window(widget), > > CAIRO_CONTENT_COLOR_ALPHA, > > I'm not sure I understand this part of the change. Here we abstract away that the widget is a GTK+ widget and then immediately call gtk_widget_get_window on it. Yes, that was a strange code and they did not have to use PlatformWidget. So, I modified to use "GtkWidget*" instead of PlatformWidget in the gtk port's .cpp files.
Comment on attachment 97925 [details] Patch for comments View in context: https://bugs.webkit.org/attachment.cgi?id=97925&action=review Okay. Are you a committer. Can you make the following change before committing? > Source/WebCore/platform/graphics/cairo/CairoUtilities.h:50 > +void copyRectFromOneSurfaceToAnother(cairo_surface_t*, cairo_surface_t*, const IntSize&, const IntRect&); Here it makes sense to leave the argument names since it's unclear which is the destination and which is the source.
(In reply to comment #10) > (From update of attachment 97925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97925&action=review > > Okay. Are you a committer. Can you make the following change before committing? Ok, Eunmi is not committer yet. I can do that. I will give cq+ when Eunmi upload new patch according to your comment.
Created attachment 98125 [details] patch for Martin's comments
(In reply to comment #10) > (From update of attachment 97925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97925&action=review > > Okay. Are you a committer. Can you make the following change before committing? > > > Source/WebCore/platform/graphics/cairo/CairoUtilities.h:50 > > +void copyRectFromOneSurfaceToAnother(cairo_surface_t*, cairo_surface_t*, const IntSize&, const IntRect&); > > Here it makes sense to leave the argument names since it's unclear which is the destination and which is the source. Thank you very much for your review :) I've added argument names for copyRectFromOneSurfaceToAnother. And, I'm not a committer, so Gyuyoung will commit this patch. Thanks for Gyuyoung :)
Comment on attachment 98125 [details] patch for Martin's comments LGTM.
Comment on attachment 98125 [details] patch for Martin's comments Clearing flags on attachment: 98125 Committed r89442: <http://trac.webkit.org/changeset/89442>
All reviewed patches have been landed. Closing bug.