[webkit-reviews] review granted: [Bug 41779] [GTK] Implement ImageDiff and add it to the build system : [Attachment 60764] ImageDiff implementation

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 41779: [GTK] Implement ImageDiff and add it to the build system
https://bugs.webkit.org/show_bug.cgi?id=41779

Attachment 60764: ImageDiff implementation
https://bugs.webkit.org/attachment.cgi?id=60764&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 102	 void* diffBuffer = malloc(width * height);
[...]
 110	 unsigned char* diff = static_cast<unsigned char*>(diffBuffer);
[...]
143	    *differenceImage =
differenceImageFromDifferenceBuffer(static_cast<unsigned char*>(diffBuffer),
width, height);

Given that you never use the diffBuffer as a void*, I would prefer having the
cast in the malloc call, it makes it a bit easier on the reader of the code.

I find the mixed usage of g_print, fputs and fprintf unnecessary, though I do
not oppose to it. I feel like you cal also make the ifs that are under the else
ifs here be just one more condition of the else if:

 206		 if (imageSize > 0 && !actualImage) {
 207		     if (!(actualImage = readPixbufFromStdin(imageSize))) {
 208			 fputs("Error, could not read actual image.\n",
stdout);
 209			 return 1;
 210		     }
 211		 } else if (imageSize > 0 && !baselineImage) {
 212		     if (!(baselineImage = readPixbufFromStdin(imageSize))) {
 213			 fputs("Error, could not read baseline image.\n",
stdout);
 214			 return 1;
 215		     }

It will make it a bit easier to read, IMO. Other than the above comments, I
think the code looks right, let's start getting some experience with how well
pixel tests will work for us, I guess =)