Bug 63502 – [EFL] Move GtkWidgetBackingStoreCairo to the cairo directory and modify to use in the EFL.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Eunmi Lee]

I've added EflWidgetBackingStoreCairo, and it will be used in the WebKit2 EFL's BackingStore.

[Eunmi Lee]

Created attachment 98845 [details]
Patch

[Gyuyoung Kim]

LGTM. CC'ing Rafael.

[Kenneth Rohde Christiansen]

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?

[Raphael Kubo da Costa (:rakuco)]

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

[Lucas De Marchi]

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

[Antonio Gomes]

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

[Gyuyoung Kim]

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

[Eric Seidel (no email)]

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.

[Eunmi Lee]

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.

[Eunmi Lee]

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

[Eunmi Lee]

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

[Kenneth Rohde Christiansen]

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

[Eunmi Lee]

Created attachment 107977 [details]
Add EflWidgetBackingStoreCairo.

Remove unnecessary private keyword.

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

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?

[Martin Robinson]

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

[Raphael Kubo da Costa (:rakuco)]

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?

[Gyuyoung Kim]

(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

[Eunmi Lee]

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

[Lucas De Marchi]

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

[Gyuyoung Kim]

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

[Eunmi Lee]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[WebKit Review Bot]

Comment on attachment 108266 [details]
Add EflWidgetBackingStoreCairo.

Rejecting attachment 108266 [details] from commit-queue.

[e-mail] 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.

[Martin Robinson]

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.

[Eunmi Lee]

Created attachment 108437 [details]
Patch for WidgetBackingStoreCairo

I moved gtk/GtkWidgetBackingStoreCairo.cpp to the cairo/WidgetBackingStoreCairo.cpp
to share the WidgetBackingStoreCairo with EFL.

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Eunmi Lee]

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

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Eunmi Lee]

Created attachment 108438 [details]
Patch for WidgetBackingStoreCairo

I created changelog again.

[Raphael Kubo da Costa (:rakuco)]

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

[Eunmi Lee]

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

[Raphael Kubo da Costa (:rakuco)]

Right, informal r+ then.

[Martin Robinson]

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

[Eunmi Lee]

Created attachment 108620 [details]
Patch for WidgetBackingStoreCairo

[Eunmi Lee]

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

[Gyuyoung Kim]

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.

[Eunmi Lee]

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

[Gyuyoung Kim]

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.

[WebKit Review Bot]

Comment on attachment 108620 [details]
Patch for WidgetBackingStoreCairo

Clearing flags on attachment: 108620

Committed r95935: <http://trac.webkit.org/changeset/95935>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.