[webkit-reviews] review denied: [Bug 85299] [GTK] Remove the GDK dependency from ImageDiff : [Attachment 167082] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied José Dapena Paz
<jdapena at igalia.com>'s request for review:
Bug 85299: [GTK] Remove the GDK dependency from ImageDiff
https://bugs.webkit.org/show_bug.cgi?id=85299

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167082&action=review


Okay. This seems like a good first step. The next one is to remove the GLib
dependency and to start sharing this code with EFL and maybe even WinCairo. A
few nits follow.

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:46
> +cairo_status_t cairoStdinReader(void *closure, unsigned char* data, unsigned
length)

Nit: The asterisk for closure are in the wrong place here.

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:48
> +    int* pendingImageSize = reinterpret_cast<int*>(closure);

I think this should be static_cast instead of reinterpret_cast.

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:49
> +    if (*pendingImageSize > 0) {

You should use an early return here:

if (*pendingImageSize <= 0)
    return CAIRO_STATUS_SUCCESS;

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:50
> +	   size_t bytesToRead = min<int>(*pendingImageSize, length);

Probably you should use min<size_t> since I think using min<int> will cast
length to a signed integer.

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:166
> +    GArray* array = reinterpret_cast<GArray*>(closure);

You should be able to use static_cast here.

> Tools/DumpRenderTree/gtk/ImageDiff.cpp:173
> +    GArray* array = g_array_new(false, false, 1);

Since g_array_new takes gboolean instead of bool, it's slightly more correct to
write FALSE here.