Bug 232225

Summary: The script should decide when an image diff passes, not ImageDiff
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, gsnedders, jbedard, mrobinson, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 232222    
Bug Blocks: 232288, 232294    
Attachments:
Description Flags
Patch none

Description Simon Fraser (smfr) 2021-10-24 21:31:21 PDT
Currently the ImageDiff binary determines when an image diff passes (diff is less than the tolerance), but it makes more sense for webkitpy to make that decision.
Comment 1 Simon Fraser (smfr) 2021-10-25 21:21:32 PDT
Created attachment 442462 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-10-25 22:53:26 PDT
Comment on attachment 442462 [details]
Patch

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

> Tools/ChangeLog:3
> +        The script should decide when an image diff cases, not ImageDiff

passes, not cases
Comment 3 Martin Robinson 2021-10-26 04:23:18 PDT
Comment on attachment 442462 [details]
Patch

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

This is a nice change.

> Tools/ImageDiff/ImageDiff.cpp:71
> +    fprintf(stdout, "diff: %01.8f%%\n", differenceData.percentageDifference);

This is a nice simplification.

> Tools/Scripts/webkitpy/port/image_diff.py:137
> +            return ImageDiffResult(passed=False, diff_image=None, difference=0, tolerance=self._tolerance, error_string=err_str or "Failed to match ImageDiff output %s" % diff_output)

I think using 

   "Failed to match ImageDiff output {}".format(diff_output)

would be better for future-proofing this code against changes to Python.
Comment 4 EWS 2021-10-26 10:39:26 PDT
Committed r284878 (243555@main): <https://commits.webkit.org/243555@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442462 [details].
Comment 5 Radar WebKit Bug Importer 2021-10-26 10:40:21 PDT
<rdar://problem/84670441>