[webkit-reviews] review denied: [Bug 58242] [GTK] Implement pixel dump support for WebKitTestRunner : [Attachment 97162] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 58242: [GTK] Implement pixel dump support for WebKitTestRunner
https://bugs.webkit.org/show_bug.cgi?id=58242

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review

> Source/WebKit2/ChangeLog:12
> +	   * Shared/API/c/cairo/WKImageCairo.h: Copied from
Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp.

This file seems to be missing from your patch.

> Tools/WebKitTestRunner/TestInvocation.h:50
> +    bool compareActualHashToExpectedAndDumpResults(char[33]);

Can the argument be a const char[33]?

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:49
> +    for (unsigned row = 0; row < pixelsHigh; row++) {

Might as well use size_t and preincrement here.

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50
> +	   md5Context.addBytes(bitmapData, 4 * pixelsWide);

Is it worth checking the bits-per-pixel of the surface instead of assuming 32?

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:92
> +    cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage);

Can you use a RefPtr here?

> Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:-145
> -	   // FIXME: Do case insensitive compare.

You lost this FIXME when moving this code.