Bug 62848 - Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the EFL port.
Summary: Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-17 01:23 PDT by Eunmi Lee
Modified: 2011-06-22 09:57 PDT (History)
14 users (show)

See Also:


Attachments
Patch (14.18 KB, patch)
2011-06-17 04:53 PDT, Eunmi Lee
gustavo: commit-queue-
Details | Formatted Diff | Diff
Change GtkWidgetBackingStore.h to WidgetBackingStore.h in order to use in the EFL port. (17.50 KB, patch)
2011-06-17 07:06 PDT, Eunmi Lee
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch to fix efl error (17.41 KB, patch)
2011-06-19 21:05 PDT, Eunmi Lee
mrobinson: review-
Details | Formatted Diff | Diff
Patch for comments (17.67 KB, patch)
2011-06-20 21:31 PDT, Eunmi Lee
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff
patch for Martin's comments (17.70 KB, patch)
2011-06-21 23:57 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2011-06-17 01:23:24 PDT
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
Comment 1 Eunmi Lee 2011-06-17 04:53:36 PDT
Created attachment 97582 [details]
Patch
Comment 2 Eunmi Lee 2011-06-17 05:00:23 PDT
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 3 Gustavo Noronha (kov) 2011-06-17 06:32:22 PDT
Comment on attachment 97582 [details]
Patch

Attachment 97582 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8880568
Comment 4 Eunmi Lee 2011-06-17 07:06:47 PDT
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 5 Gyuyoung Kim 2011-06-17 16:15:16 PDT
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
Comment 6 Eunmi Lee 2011-06-19 21:05:41 PDT
Created attachment 97740 [details]
Patch to fix efl error

remove EFL related codes to fix efl build break.
Comment 7 Martin Robinson 2011-06-20 11:06:40 PDT
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.
Comment 8 Eunmi Lee 2011-06-20 21:31:25 PDT
Created attachment 97925 [details]
Patch for comments
Comment 9 Eunmi Lee 2011-06-20 21:41:52 PDT
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 10 Martin Robinson 2011-06-21 11:03:29 PDT
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.
Comment 11 Gyuyoung Kim 2011-06-21 22:17:11 PDT
(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.
Comment 12 Eunmi Lee 2011-06-21 23:57:57 PDT
Created attachment 98125 [details]
patch for Martin's comments
Comment 13 Eunmi Lee 2011-06-22 00:00:10 PDT
(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 14 Gyuyoung Kim 2011-06-22 00:05:57 PDT
Comment on attachment 98125 [details]
patch for Martin's comments

LGTM.
Comment 15 WebKit Review Bot 2011-06-22 09:57:20 PDT
Comment on attachment 98125 [details]
patch for Martin's comments

Clearing flags on attachment: 98125

Committed r89442: <http://trac.webkit.org/changeset/89442>
Comment 16 WebKit Review Bot 2011-06-22 09:57:27 PDT
All reviewed patches have been landed.  Closing bug.