[webkit-reviews] review denied: [Bug 58486] [WinCairo] Implement ImageDiff Logic : [Attachment 89481] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 58486: [WinCairo] Implement ImageDiff Logic
https://bugs.webkit.org/show_bug.cgi?id=58486

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

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

In general this change looks pretty good to me, though it originates from
before a lot of the current WebKit style guidelines. Across all code, I'd like
to see:

1. All C++ style casts.
2. Remove 'f' after floating point numbers when it's not necessary (when the .0
is enough).

It's a pity we don't share more of this with the GTK port, but perhaps another
patch can remove the GTK version in favor of this one.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:47
> +#include <wtf/MathExtras.h>
> +
> +#include <fcntl.h>
> +#include <io.h>
> +#include <winsock2.h>
> +#include <windows.h>
> +#endif

If possible please fix these style errors and if not leave a comment
documenting why certain headers must be in an unusual order.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:60
> +    CFMutableDataRef cfDataObj = (CFMutableDataRef)closure;

Please use a C++ style cast here and complete words for variable names.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:79
> +    return cairo_image_surface_create_from_png_stream
((cairo_read_func_t)readFromData, data.get());

Please use C++ style cast here as well.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:113
> +    // Draw base image in bitmap context
> +    void* baseBuffer = calloc(height, rowBytes);
> +    cairo_surface_t* baseImageStore =
cairo_image_surface_create_for_data((unsigned char*)baseBuffer,
CAIRO_FORMAT_ARGB32, width, height, width * 4); 
> +    cairo_t* baseContext = cairo_create(baseImageStore);
> +    cairo_surface_destroy(baseImageStore);
> +
> +    cairo_set_source_surface(baseContext, baseImage, 0, 0);
> +    cairo_paint(baseContext);
> +    cairo_destroy(baseContext);
> +
> +    // Draw test image in bitmap context
> +    void* buffer = calloc(height, rowBytes);
> +    cairo_surface_t* testImageStore =
cairo_image_surface_create_for_data((unsigned char*)buffer,
CAIRO_FORMAT_ARGB32, width, height, width * 4); 
> +    cairo_t* testContext = cairo_create(testImageStore);
> +    cairo_surface_destroy(testImageStore);
> +
> +    cairo_set_source_surface(testContext, testImage, 0, 0);
> +    cairo_paint(testContext);
> +    cairo_destroy(testContext);

Why redraw the images again here? Can't you just do cairo_imag_surface_get_data
on baseImage and testImage?

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:178
> +    cairo_format_t info = cairo_image_surface_get_format(image);
> +    
> +    return (info == CAIRO_FORMAT_ARGB32);

this can just be:

return (cairo_image_surface_get_format(image) == CAIRO_FORMAT_ARGB32);

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:183
> +    CFMutableDataRef cfDataObj = (CFMutableDataRef)closure;

Ditto.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:214
> +	   // remove the CR

The comment here should be a complete sentence.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:235
> +	       if ((cairo_image_surface_get_width(actualImage) ==
cairo_image_surface_get_width(baselineImage)) &&
(cairo_image_surface_get_height(actualImage) ==
cairo_image_surface_get_height(baselineImage)) && (imageHasAlpha(actualImage)
== imageHasAlpha(baselineImage))) {

Please split this line since it's longer than the customary 120 characters.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:241
> +		       difference = 0.0f;
> +		   else {
> +		       difference = roundf(difference * 100.0f) / 100.0f;
> +		       difference = max(difference, 0.01f); // round to 2
decimal places

Style guidelines suggest that you should avoid using the 'f' after floating
point numbers.

> Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:246
> +	       if (difference > 0.0f) {

Ditto.