Bug 63502 - [EFL] Move GtkWidgetBackingStoreCairo to the cairo directory and modify to use in the EFL.
Summary: [EFL] Move GtkWidgetBackingStoreCairo to the cairo directory and modify to us...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-27 18:56 PDT by Eunmi Lee
Modified: 2011-09-25 23:52 PDT (History)
14 users (show)

See Also:


Attachments
Patch (6.27 KB, patch)
2011-06-27 19:46 PDT, Eunmi Lee
eric: review-
Details | Formatted Diff | Diff
Add EflWidgetBackingStoreCairo. (5.56 KB, patch)
2011-09-19 17:50 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Add EflWidgetBackingStoreCairo. (5.55 KB, patch)
2011-09-20 02:05 PDT, Eunmi Lee
rakuco: review-
Details | Formatted Diff | Diff
Add EflWidgetBackingStoreCairo. (10.00 KB, patch)
2011-09-21 19:08 PDT, Eunmi Lee
rakuco: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for WidgetBackingStoreCairo (11.04 KB, patch)
2011-09-22 20:28 PDT, Eunmi Lee
mrobinson: review-
Details | Formatted Diff | Diff
Patch for WidgetBackingStoreCairo (11.33 KB, patch)
2011-09-22 21:18 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch for WidgetBackingStoreCairo (12.04 KB, patch)
2011-09-25 19:13 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-27 18:56:16 PDT
I've added EflWidgetBackingStoreCairo, and it will be used in the WebKit2 EFL's BackingStore.
Comment 1 Eunmi Lee 2011-06-27 19:46:51 PDT
Created attachment 98845 [details]
Patch
Comment 2 Gyuyoung Kim 2011-06-27 19:53:57 PDT
LGTM. CC'ing Rafael.
Comment 3 Kenneth Rohde Christiansen 2011-06-28 01:19:23 PDT
Could we avoid cc'ing everyone who has reviewed an EFL patch at some point? and just cc people who have particular interest or experience in the area the patch touches?

I guess you can get an unassigned-bugs mailing list and subscribe the EFL developers to that?

Eric, do you know how to get this set up?
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-06-28 05:48:09 PDT
> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:48
> +        int w = size.width();

These two variables could be const.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:51
> +        /* get cairo_surface from widget */

Is this comment really needed? If so, could you change it to a C++ comment? Remember you need to start with a capital letter and end with a period.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:57
> +        /* create scroll surface */

Ditto.
Comment 5 Lucas De Marchi 2011-06-28 05:58:52 PDT
(In reply to comment #3)
> Could we avoid cc'ing everyone who has reviewed an EFL patch at some point? and just cc people who have particular interest or experience in the area the patch touches?
> 
> I guess you can get an unassigned-bugs mailing list and subscribe the EFL developers to that?

Yes, I think this is the right thing to do. However I'm thinking what we should do since there are no official reviewers for EFL port. Maybe CC some reviewers *after* the informal reviews?
Comment 6 Antonio Gomes 2011-06-28 16:08:53 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > Could we avoid cc'ing everyone who has reviewed an EFL patch at some point? and just cc people who have particular interest or experience in the area the patch touches?
> > 
> > I guess you can get an unassigned-bugs mailing list and subscribe the EFL developers to that?
> 
> Yes, I think this is the right thing to do. However I'm thinking what we should do since there are no official reviewers for EFL port. Maybe CC some reviewers *after* the informal reviews?

Or maybe try to poke the candidate reviewer on IRC first?
Comment 7 Gyuyoung Kim 2011-06-28 20:03:38 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Could we avoid cc'ing everyone who has reviewed an EFL patch at some point? and just cc people who have particular interest or experience in the area the patch touches?
> > > 
> > > I guess you can get an unassigned-bugs mailing list and subscribe the EFL developers to that?
> > 
> > Yes, I think this is the right thing to do. However I'm thinking what we should do since there are no official reviewers for EFL port. Maybe CC some reviewers *after* the informal reviews?
> 
> Or maybe try to poke the candidate reviewer on IRC first?

In my opinion, it is better to find reviewers on irc after the informal reviews.
Comment 8 Eric Seidel (no email) 2011-09-12 15:54:25 PDT
Comment on attachment 98845 [details]
Patch

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

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:51
> +        /* get cairo_surface from widget */

Comments in webkit begin with a capital and end with a period (full sentences).  I don't hintk this comment is helpign undrestand what's going on.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:54
> +        m_surface = adoptRef(cairo_image_surface_create_for_data
> +                             ((unsigned char*)pixels, CAIRO_FORMAT_ARGB32, w, h, w * 4));

c++ cast in a c++ file please.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:57
> +        /* create scroll surface */

Not helpful.  Also, you'd use a // comment anyway.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:86
> +    targetRect.shiftMaxXEdgeTo(targetRect.maxX() - scrollOffset.width());
> +    targetRect.shiftMaxYEdgeTo(targetRect.maxY() - scrollOffset.height());

I'm surprised there isn't a combined function which takes an IntSize?  Is this just the same as calling setSize on the rect?

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:93
> +    copyRectFromOneSurfaceToAnother(m_private->m_surface.get(), m_private->m_scrollSurface.get(),
> +                                    scrollOffset, targetRect);
> +    copyRectFromOneSurfaceToAnother(m_private->m_scrollSurface.get(), m_private->m_surface.get(),
> +                                    IntSize(), targetRect);

I'm not sure wrapping is helping readability here.
Comment 9 Eunmi Lee 2011-09-19 17:50:34 PDT
Created attachment 107949 [details]
Add EflWidgetBackingStoreCairo.

I modified to create m_surface as a pure cairo_image_surface instead of using widget's data,
because widget's data size and WidgetBackingStore's size can be different.
And I fixed wrong comment style.
Comment 10 Eunmi Lee 2011-09-19 17:53:27 PDT
(In reply to comment #4)
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:48
> > +        int w = size.width();
> 
> These two variables could be const.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:51
> > +        /* get cairo_surface from widget */
> 
> Is this comment really needed? If so, could you change it to a C++ comment? Remember you need to start with a capital letter and end with a period.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:57
> > +        /* create scroll surface */
> 
> Ditto.

Thanks for your comments :)
int w = size.width() is removed because it is not used anymore,
but I will be careful to use "const" for IntSize.
And I modified all comments to right style.
Comment 11 Eunmi Lee 2011-09-19 18:17:47 PDT
(In reply to comment #8)
> (From update of attachment 98845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98845&action=review
> 

Thank you for your comments.

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:51
> > +        /* get cairo_surface from widget */
> 
> Comments in webkit begin with a capital and end with a period (full sentences).  I don't hintk this comment is helpign undrestand what's going on.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:54
> > +        m_surface = adoptRef(cairo_image_surface_create_for_data
> > +                             ((unsigned char*)pixels, CAIRO_FORMAT_ARGB32, w, h, w * 4));
> 
> c++ cast in a c++ file please.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:57
> > +        /* create scroll surface */
> 
> Not helpful.  Also, you'd use a // comment anyway.
> 

I modified all comments in the code as your advice,
and type-casting code is removed but I will be careful to use type-casting.

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:86
> > +    targetRect.shiftMaxXEdgeTo(targetRect.maxX() - scrollOffset.width());
> > +    targetRect.shiftMaxYEdgeTo(targetRect.maxY() - scrollOffset.height());
> 
> I'm surprised there isn't a combined function which takes an IntSize?  Is this just the same as calling setSize on the rect?
> 

Yes, actually it is same with setSize with checking that size is bigger than 0.
So, I modified above two line as follows:
+    targetRect.setWidth(std::max(0, targetRect.width() - scrollOffset.width()));
+    targetRect.setHeight(std::max(0, targetRect.height() - scrollOffset.height()));

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:93
> > +    copyRectFromOneSurfaceToAnother(m_private->m_surface.get(), m_private->m_scrollSurface.get(),
> > +                                    scrollOffset, targetRect);
> > +    copyRectFromOneSurfaceToAnother(m_private->m_scrollSurface.get(), m_private->m_surface.get(),
> > +                                    IntSize(), targetRect);
> 
> I'm not sure wrapping is helping readability here.

That codes are for coping reusable area within the m_surface when scrolling.
m_surface's reusable area is copied to m_scrollSurface (temporary),
and then copied area of m_scrollSurface is copied to the m_surface again.
For that, I use the copyRectFromOneSurfaceToAnother function in the CairoUtilities.cpp.
Do you think it is better to use cairo operation directly?
Comment 12 Kenneth Rohde Christiansen 2011-09-20 01:13:13 PDT
Comment on attachment 98845 [details]
Patch

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

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:42
> +private:

When you make a private class it is common to have everything just public as it is protected anyway.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:49
> +        int w = size.width();
> +        int h = size.height();

We normally write things out, ie. width, height and not w, h

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:81
> +void WidgetBackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)

Maybe const IntRect& areaToScroll, const IntSize& delta) ?
Comment 13 Eunmi Lee 2011-09-20 02:05:57 PDT
Created attachment 107977 [details]
Add EflWidgetBackingStoreCairo.

Remove unnecessary private keyword.
Comment 14 Eunmi Lee 2011-09-20 02:12:13 PDT
(In reply to comment #12)
> (From update of attachment 98845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98845&action=review
> 

Kenneth, thanks for your review :)

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:42
> > +private:
> 
> When you make a private class it is common to have everything just public as it is protected anyway.

Ok, you are right, so I removed private keyword.

> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:49
> > +        int w = size.width();
> > +        int h = size.height();
> 
> We normally write things out, ie. width, height and not w, h
>

That codes are not used anymore, but I will be careful to use full word for variable's name.
 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:81
> > +void WidgetBackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)
> 
> Maybe const IntRect& areaToScroll, const IntSize& delta) ?

Yes, scrollRect means areaToScroll and scrollOffset means delta of scroll.
But, scroll function will be used at WebKit2's BackingStore and its parameters will be filled with updateInfo.scrollRect and updateInfo.scrollOffset.
So, I think current parameter name will be useful.
Comment 15 Raphael Kubo da Costa (:rakuco) 2011-09-20 07:39:30 PDT
This code is 99% equal to WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp, the only difference in functionality is in the code to set m_surface and m_scrollSurface.

mrobinson, wouldn't it make sense to move this code to cairo/WidgetBackingStore.cpp, leaving only the surface creation to each platform?
Comment 16 Martin Robinson 2011-09-20 15:16:41 PDT
(In reply to comment #15)
> This code is 99% equal to WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp, the only difference in functionality is in the code to set m_surface and m_scrollSurface.
> 
> mrobinson, wouldn't it make sense to move this code to cairo/WidgetBackingStore.cpp, leaving only the surface creation to each platform?

This was one of the reasons I originally made a private data section for this class. Can't we just put the common code in cairo/WidgetBackingStoreCairo.cpp and then move the private data structures to platform-dependent files?
Comment 17 Raphael Kubo da Costa (:rakuco) 2011-09-20 21:59:19 PDT
Comment on attachment 107977 [details]
Add EflWidgetBackingStoreCairo.

Yeah, I also think this is the way to go, so I'm r-'ing this patch.

Eunmi, could you please try to work on that direction and submit a new patch?
Comment 18 Gyuyoung Kim 2011-09-20 22:05:56 PDT
(In reply to comment #17)
> (From update of attachment 107977 [details])
> Yeah, I also think this is the way to go, so I'm r-'ing this patch.
> 

As far as I know, we should not set r+ nor r- in unofficial review.

" Note that you should not put r+ nor r- on patches in such unofficial reviews."
http://www.webkit.org/coding/commit-review-policy.html
Comment 19 Eunmi Lee 2011-09-20 22:12:26 PDT
(In reply to comment #17)
> (From update of attachment 107977 [details])
> Yeah, I also think this is the way to go, so I'm r-'ing this patch.
> 
> Eunmi, could you please try to work on that direction and submit a new patch?

Ok, I agree with you and mrobinson.
I will make a new patch soon. :)
Comment 20 Lucas De Marchi 2011-09-21 06:25:46 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 107977 [details] [details])
> > Yeah, I also think this is the way to go, so I'm r-'ing this patch.
> > 
> 
> As far as I know, we should not set r+ nor r- in unofficial review.
> 
> " Note that you should not put r+ nor r- on patches in such unofficial reviews."
> http://www.webkit.org/coding/commit-review-policy.html

This is not how things are working. Since official reviewers want our informal r+, letting it as "r?" will only make a lot of bugs appear as unreviewed on  "webkit-patch patches-to-review". And this is what Eric Seidel, for e.g., uses to review EFL bug requests. In fact he said me it's fine for informal-reviewers to set r- after we had some problems with him rubber-stamping bugs that were already informally r- but without removing the "r?".
Comment 21 Gyuyoung Kim 2011-09-21 18:34:18 PDT
(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 107977 [details] [details] [details])
> > > Yeah, I also think this is the way to go, so I'm r-'ing this patch.
> > > 
> > 
> > As far as I know, we should not set r+ nor r- in unofficial review.
> > 
> > " Note that you should not put r+ nor r- on patches in such unofficial reviews."
> > http://www.webkit.org/coding/commit-review-policy.html
> 
> This is not how things are working. Since official reviewers want our informal r+, letting it as "r?" will only make a lot of bugs appear as unreviewed on  "webkit-patch patches-to-review". And this is what Eric Seidel, for e.g., uses to review EFL bug requests. In fact he said me it's fine for informal-reviewers to set r- after we had some problems with him rubber-stamping bugs that were already informally r- but without removing the "r?".

If reviewers said that is ok, I'm no problem, thank you.
Comment 22 Eunmi Lee 2011-09-21 19:08:14 PDT
Created attachment 108266 [details]
Add EflWidgetBackingStoreCairo.

I've created platform/cairo/WidgetBackingStoreCairo.cpp and moved common code of Efl and Gtk there.
Only WidgetBackingStorePrivate related code is remained in the each platform's WidgetBackingStoreCairo.
Comment 23 Raphael Kubo da Costa (:rakuco) 2011-09-21 20:57:09 PDT
Comment on attachment 108266 [details]
Add EflWidgetBackingStoreCairo.

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

There's more code to share here. I really meant it when I said 99% of the files were equal. The only difference between EflWidgetBackingStoreCairo and GtkWidgetBackingStoreCairo is that the former uses cairo_image_surface_create and the latter uses gdk_window_create_similar_surface to set WidgetBackingStorePrivate::{m_surface,m_scrollSurface}.

Please try to factor out even more common code.

> Source/WebCore/ChangeLog:20
> +        * platform/cairo/WidgetBackingStoreCairo.cpp: Added.

To me, it makes more sense to name it WidgetBackingStore.cpp to match its header.

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:39
> +    targetRect.setWidth(std::max(0, targetRect.width() - scrollOffset.width()));

targetRect.shiftMaxXEdgeTo() looks cleaner -- Eric's question in comment #8 was really a question, not a request for you to change the code.

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:40
> +    targetRect.setHeight(std::max(0, targetRect.height() - scrollOffset.height()));

Ditto.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:44
> +    WidgetBackingStorePrivate(const IntSize& size)

I prefer gtk's approach of setting m_surface and m_scrollSurface in the initialization list.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:53
> +WidgetBackingStore::WidgetBackingStore(Evas_Object* widget, const IntSize& size)

widget is not being used, and its type should be PlatformWidget.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:62
> +cairo_surface_t* WidgetBackingStore::cairoSurface()

This is common code.

> Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:67
> +void WidgetBackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)

This is common code.

> Source/WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp:62
>  WidgetBackingStore::WidgetBackingStore(GtkWidget* widget, const IntSize& size)

widget's type could be changed to PlatformWidget.
Comment 24 WebKit Review Bot 2011-09-21 20:57:36 PDT
Comment on attachment 108266 [details]
Add EflWidgetBackingStoreCairo.

Rejecting attachment 108266 [details] from commit-queue.

kubo@profusion.mobi does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 25 Martin Robinson 2011-09-21 21:08:48 PDT
Comment on attachment 108266 [details]
Add EflWidgetBackingStoreCairo.

I agree that much more of this code could be shared. It seems like you could create PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(GtkWidget* widget, const IntSize& size)  for each both GTK+ and EFL and share the rest.
Comment 26 Eunmi Lee 2011-09-22 20:28:39 PDT
Created attachment 108437 [details]
Patch for WidgetBackingStoreCairo

I moved gtk/GtkWidgetBackingStoreCairo.cpp to the cairo/WidgetBackingStoreCairo.cpp
to share the WidgetBackingStoreCairo with EFL.
Comment 27 Eunmi Lee 2011-09-22 20:45:05 PDT
(In reply to comment #23)
> (From update of attachment 108266 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108266&action=review
> 

Thank you for your detailed comments!
Now, I've moved every duplicated codes to the WidgetBackingStoreCairo.

> There's more code to share here. I really meant it when I said 99% of the files were equal. The only difference between EflWidgetBackingStoreCairo and GtkWidgetBackingStoreCairo is that the former uses cairo_image_surface_create and the latter uses gdk_window_create_similar_surface to set WidgetBackingStorePrivate::{m_surface,m_scrollSurface}.
> 
> Please try to factor out even more common code.
> 
> > Source/WebCore/ChangeLog:20
> > +        * platform/cairo/WidgetBackingStoreCairo.cpp: Added.
> 
> To me, it makes more sense to name it WidgetBackingStore.cpp to match its header.

I think it is better to use WidgetBackingStoreCairo.cpp in order to distinguish with GtkWidgetBackingStoreX11.cpp. Both of them use WidgetBackingStore.h.

> 
> > Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:39
> > +    targetRect.setWidth(std::max(0, targetRect.width() - scrollOffset.width()));
> 
> targetRect.shiftMaxXEdgeTo() looks cleaner -- Eric's question in comment #8 was really a question, not a request for you to change the code.
> 
> > Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:40
> > +    targetRect.setHeight(std::max(0, targetRect.height() - scrollOffset.height()));
> 
> Ditto.
>

There was a misunderstanding because of my limited English.
So, I leaved shiftMaxXEdgeTo for readability.

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:44
> > +    WidgetBackingStorePrivate(const IntSize& size)
> 
> I prefer gtk's approach of setting m_surface and m_scrollSurface in the initialization list.
> 

I integrated with gtk's approach.

> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:53
> > +WidgetBackingStore::WidgetBackingStore(Evas_Object* widget, const IntSize& size)
> 
> widget is not being used, and its type should be PlatformWidget.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:62
> > +cairo_surface_t* WidgetBackingStore::cairoSurface()
> 
> This is common code.
> 
> > Source/WebCore/platform/efl/EflWidgetBackingStoreCairo.cpp:67
> > +void WidgetBackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)
> 
> This is common code.
> 

All common codes have been integrated.

> > Source/WebCore/platform/gtk/GtkWidgetBackingStoreCairo.cpp:62
> >  WidgetBackingStore::WidgetBackingStore(GtkWidget* widget, const IntSize& size)
> 
> widget's type could be changed to PlatformWidget.

I used PlatformWidget in the cairo/WidgetBackingStoreCairo.cpp.
Comment 28 Raphael Kubo da Costa (:rakuco) 2011-09-22 20:45:57 PDT
Comment on attachment 108437 [details]
Patch for WidgetBackingStoreCairo

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

Informal r+ from my side once you answer the question below, the patch looks much better now.

My only concern is the way you created WidgetbackingStoreCairo.cpp -- did you properly copy GtkWidgetBackingStore.cpp and then changed it? Preserving history across renames is very important.

> Source/WebCore/ChangeLog:17
> +        * platform/cairo/WidgetBackingStoreCairo.cpp: Added.

Shouldn't this say 'Copied from foo/bar.cpp'? Make sure you use svn cp (or the git equivalent here).
Comment 29 Eunmi Lee 2011-09-22 20:47:49 PDT
(In reply to comment #25)
> (From update of attachment 108266 [details])
> I agree that much more of this code could be shared. It seems like you could create PassRefPtr<cairo_surface_t> createSurfaceForBackingStore(GtkWidget* widget, const IntSize& size)  for each both GTK+ and EFL and share the rest.

Thanks for your comment.
I've integrated all codes in the cairo/WidgetBackingStoreCairo.cpp.
Comment 30 Eunmi Lee 2011-09-22 20:51:09 PDT
(In reply to comment #28)
> (From update of attachment 108437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108437&action=review
> 
> Informal r+ from my side once you answer the question below, the patch looks much better now.
> 
> My only concern is the way you created WidgetbackingStoreCairo.cpp -- did you properly copy GtkWidgetBackingStore.cpp and then changed it? Preserving history across renames is very important.
> 
> > Source/WebCore/ChangeLog:17
> > +        * platform/cairo/WidgetBackingStoreCairo.cpp: Added.
> 
> Shouldn't this say 'Copied from foo/bar.cpp'? Make sure you use svn cp (or the git equivalent here).

Yes, it is better to understand history of change. I will modify changelog as your comment :)
Comment 31 Raphael Kubo da Costa (:rakuco) 2011-09-22 20:53:10 PDT
(In reply to comment #30)
> (In reply to comment #28)
> > > Source/WebCore/ChangeLog:17
> > > +        * platform/cairo/WidgetBackingStoreCairo.cpp: Added.
> > 
> > Shouldn't this say 'Copied from foo/bar.cpp'? Make sure you use svn cp (or the git equivalent here).
> 
> Yes, it is better to understand history of change. I will modify changelog as your comment :)

My point is that the tools you used to generate this patch (webkit-patch or prepare-ChangeLog) should have done this automatically. If they have not, I fear the commit bot will just svn rm GtkWidgetBackingStoreCairo.cpp and svn add WidgetBackingStoreCairo.cpp instead of using svn mv (I'm not sure, though).
Comment 32 Eunmi Lee 2011-09-22 21:18:18 PDT
Created attachment 108438 [details]
Patch for WidgetBackingStoreCairo

I created changelog again.
Comment 33 Raphael Kubo da Costa (:rakuco) 2011-09-22 21:22:21 PDT
(In reply to comment #32)
> Created an attachment (id=108438) [details]
> Patch for WidgetBackingStoreCairo
> 
> I created changelog again.

Was the "Renamed from" part automatically generated (ie. you did a proper svn mv when creating the patch)?
Comment 34 Eunmi Lee 2011-09-22 21:27:36 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > Created an attachment (id=108438) [details] [details]
> > Patch for WidgetBackingStoreCairo
> > 
> > I created changelog again.
> 
> Was the "Renamed from" part automatically generated (ie. you did a proper svn mv when creating the patch)?

Yes, it was automatically generated.

I reused changelog of old patch and modify that by hand, and it was a problem.
So, I made patch again in the scratch.
I did "git mv gtk/GtkWidgetBackingStoreCairo.cpp cairo/WidgetBackingStoreCairo.cpp" and then modified all codes again.
Finally, I could get new ChangeLog using "Tools/Scripts/prepare-ChangeLog".

I'm using git, so I use "git mv" because there is no "git copy".
Comment 35 Raphael Kubo da Costa (:rakuco) 2011-09-22 21:32:33 PDT
Right, informal r+ then.
Comment 36 Martin Robinson 2011-09-23 06:35:13 PDT
Comment on attachment 108437 [details]
Patch for WidgetBackingStoreCairo

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

Much nicer, but please fix the minor stuff below. :)

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:23
> +#if (PLATFORM(GTK) && !defined(XP_UNIX)) || PLATFORM(EFL)

You shouldn't need this line. If a platform doesn't want to compile this file, they just won't have it on their source list.

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:45
> +#elif PLATFORM(EFL)
> +    return adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(), size.height()));
> +#else
> +    return nullptr;
> +#endif

It would be better to simply have

#if PLATFORM(GTK)
...
#else
    return adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(), size.height()));
#endif
Comment 37 Eunmi Lee 2011-09-25 19:13:47 PDT
Created attachment 108620 [details]
Patch for WidgetBackingStoreCairo
Comment 38 Eunmi Lee 2011-09-25 19:18:15 PDT
(In reply to comment #36)
> (From update of attachment 108437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108437&action=review
> 
> Much nicer, but please fix the minor stuff below. :)
> 

Thanks for your comments again :)

> > Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:23
> > +#if (PLATFORM(GTK) && !defined(XP_UNIX)) || PLATFORM(EFL)
> 
> You shouldn't need this line. If a platform doesn't want to compile this file, they just won't have it on their source list.
> 

Ok, so I removed that line and modified GNUmakefile.list.am to add GtkWidgetBackingStoreX11.cpp for TARGET_X11 and WidgetBackingStoreCairo.cpp for TARGET_WIN32.

> > Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:45
> > +#elif PLATFORM(EFL)
> > +    return adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(), size.height()));
> > +#else
> > +    return nullptr;
> > +#endif
> 
> It would be better to simply have
> 
> #if PLATFORM(GTK)
> ...
> #else
>     return adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(), size.height()));
> #endif

Done. I modified as your comment.
Comment 39 Gyuyoung Kim 2011-09-25 19:19:45 PDT
Comment on attachment 108620 [details]
Patch for WidgetBackingStoreCairo

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

> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:94
> +        return;

Despite of trivial nit, it seems there is an unneeded space in front of return keyword.
Comment 40 Eunmi Lee 2011-09-25 19:26:10 PDT
(In reply to comment #39)
> (From update of attachment 108620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108620&action=review
> 
> > Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:94
> > +        return;
> 
> Despite of trivial nit, it seems there is an unneeded space in front of return keyword.

There are 8 spaces in front of 'return'. What is wrong?
Comment 41 Gyuyoung Kim 2011-09-25 19:32:13 PDT
Comment on attachment 108620 [details]
Patch for WidgetBackingStoreCairo

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

>>> Source/WebCore/platform/cairo/WidgetBackingStoreCairo.cpp:94
>>> +        return;
>> 
>> Despite of trivial nit, it seems there is an unneeded space in front of return keyword.
> 
> There are 8 spaces in front of 'return'. What is wrong?

Sorry, there is no problem. I mistake.
Comment 42 WebKit Review Bot 2011-09-25 23:52:20 PDT
Comment on attachment 108620 [details]
Patch for WidgetBackingStoreCairo

Clearing flags on attachment: 108620

Committed r95935: <http://trac.webkit.org/changeset/95935>
Comment 43 WebKit Review Bot 2011-09-25 23:52:28 PDT
All reviewed patches have been landed.  Closing bug.