[webkit-reviews] review granted: [Bug 58486] [WinCairo] Implement ImageDiff Logic : [Attachment 89623] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted 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 89623: Patch
https://bugs.webkit.org/attachment.cgi?id=89623&action=review

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

Looks good. Please consider my suggestions before landing.

> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:58
> +static inline float strtof(const char *nptr, char **endptr)
> +{
> +    return strtod(nptr, endptr);
> +}
> +#endif

Looks like the fixes here were lost? Please fix before landing. :)

> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:91
> +static cairo_user_data_key_t imageDataKey;

Should give this the same naming convention as the statics above
s_imageDataKey.

> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:113
> +    unsigned char* basePixel = cairo_image_surface_get_data(baseImage);
> +    unsigned char* pixel = cairo_image_surface_get_data(testImage);

I think something like testPixel would be clearer here. Or even actualPixels
and expectedPixels. :)

> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:161
> +    if (difference > 0) {
> +	   normalizeBuffer(maxDistance, reinterpret_cast<unsigned
char*>(diffBuffer), height * width);
> +	   
> +	   diffImage = cairo_image_surface_create_for_data(diffPixel,
CAIRO_FORMAT_ARGB32, width, height, width * s_bytesPerPixel); 
> +	   cairo_surface_set_user_data(diffImage, &imageDataKey, diffBuffer,
releaseMallocBuffer);
> +    } else
> +	   free(diffBuffer);
> +    
> +    return diffImage;

I think an early return would be clearer here: 

if (difference <= 0) {
    free(diffBuffer);
    return 0;
}

normalizeBuffer(maxDistance, reinterpret_cast<unsigned char*>(diffBuffer),
height * width);
cairo_surface_t* diffImage = cairo_image_surface_create_for_data(diffPixel,
CAIRO_FORMAT_ARGB32, width, height, width * s_bytesPerPixel);
cairo_surface_set_user_data(diffImage, &imageDataKey, diffBuffer,
releaseMallocBuffer);
return diffImage;