[webkit-reviews] review denied: [Bug 63502] [EFL] Add EflWidgetBackingStoreCairo : [Attachment 98845] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied EunMi Lee <eunmi15.lee at samsung.com>'s
request for review:
Bug 63502: [EFL] Add EflWidgetBackingStoreCairo
https://bugs.webkit.org/show_bug.cgi?id=63502

Attachment 98845: Patch
https://bugs.webkit.org/attachment.cgi?id=98845&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.