[webkit-reviews] review denied: [Bug 63502] [EFL] Add EflWidgetBackingStoreCairo : [Attachment 108266] Add EflWidgetBackingStoreCairo.

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> 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 108266: Add EflWidgetBackingStoreCairo.
https://bugs.webkit.org/attachment.cgi?id=108266&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
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.