Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the EFL port.

• Component: WebKit2
• Status: RESOLVED FIXED

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 97582 [details]
Patch

[Eunmi Lee]

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

[Gustavo Noronha (kov)]

Comment on attachment 97582 [details]
Patch

Attachment 97582 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8880568

[Eunmi Lee]

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

[Gyuyoung Kim]

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

[Eunmi Lee]

Created attachment 97740 [details]
Patch to fix efl error

remove EFL related codes to fix efl build break.

[Martin Robinson]

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.

[Eunmi Lee]

Created attachment 97925 [details]
Patch for comments

[Eunmi Lee]

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.

[Martin Robinson]

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.

[Gyuyoung Kim]

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

[Eunmi Lee]

Created attachment 98125 [details]
patch for Martin's comments

[Eunmi Lee]

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

[Gyuyoung Kim]

Comment on attachment 98125 [details]
patch for Martin's comments

LGTM.

[WebKit Review Bot]

Comment on attachment 98125 [details]
patch for Martin's comments

Clearing flags on attachment: 98125

Committed r89442: <http://trac.webkit.org/changeset/89442>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.