[webkit-reviews] review granted: [Bug 232522] ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images : [Attachment 442897] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 232522: ImageDiff no longer needs a --tolerance argument, and fix
sometimes-black diff images
https://bugs.webkit.org/show_bug.cgi?id=232522

Attachment 442897: Patch

https://bugs.webkit.org/attachment.cgi?id=442897&action=review




--- Comment #3 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 442897
  --> https://bugs.webkit.org/attachment.cgi?id=442897
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442897&action=review

Looks good. Just a few suggestions before landing.

> Tools/ImageDiff/ImageDiff.cpp:111
> +		   "usage: ImageDiff [-h] [-v] [-d] [-e] [-t TOLERANCE]
([actualImage baselineImage] | <stdin>)\n" \

I think you should remove `[-t TOLERANCE]` from this line now as well.

> Tools/ImageDiff/ImageDiff.cpp:123
>		   "  -t, --tolerance TOLERANCE\n" \
>		   "			    compare the images with the given
tolerance\n"

These two lines should be removed as well.

> Tools/ImageDiff/PlatformImage.cpp:101
> +	       float pixel = static_cast<float>(diffPixel[p]);

Is it possible that this cast is unnecessary?

> Tools/ImageDiff/PlatformImage.cpp:105
> +	       diffPixel[p] = static_cast<unsigned char>(pixel);

Ditto.