Bug 232010 – Add support for WPT's per-test reftest fuzzy specification to WebKitTestRunner

• Component: Tools / Tests
• Status: RESOLVED LATER

[Martin Robinson]

https://web-platform-tests.org/writing-tests/reftests.html describes the WPT's per-test fuzziness specification of the form:

  <meta name=fuzzy content="maxDifference=15;totalPixels=300">

This bug tracks adding basic support for this.

[Martin Robinson]

Created attachment 441878 [details]
Patch

[Darin Adler]

Comment on attachment 441878 [details]
Patch

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

Looks good.

The test failures seen on EWS seem like they could be related to this change. I won’d say review+ until we understand that.

> Tools/ImageDiff/ImageDiff.cpp:183
> +                float difference = roundf((*result).percentageDifference * 100.0f) / 100.0f;

I suggest using std::round here instead of roundf. Both are the same function with a different name.

> Tools/ImageDiff/ImageDiff.cpp:184
>                  difference = std::max<float>(difference, 0.01f); // round to 2 decimal places

No need for the <float> here.

> Tools/ImageDiff/PlatformImage.h:61
> +        float percentageDifference;
> +        unsigned long pixelsDifferent;
> +        unsigned long maxChannelDifference;

Consider initializing these all to zero here?

> Tools/TestRunnerShared/ReftestFunctions.h:28
> +#include <wtf/text/WTFString.h>

Only Forward.h is needed to compile a function with String as a return value.

> Tools/TestRunnerShared/ReftestFunctions.h:36
> +String getFuzzyMatchingTolerance(JSGlobalContextRef);

WebKit coding style says we don’t name functions like this with the word "get". Is String really the type we want to use for strings here?

[Martin Robinson]

Created attachment 442002 [details]
Patch

[Martin Robinson]

Created attachment 442007 [details]
Patch

[Martin Robinson]

(In reply to Darin Adler from comment #2)
> Comment on attachment 441878 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441878&action=review
> 
> Looks good.

Thank you for the review.
 
> The test failures seen on EWS seem like they could be related to this
> change. I won’d say review+ until we understand that.

The WebKit 1 failures were due to a lack of an implementation in DumpRenderTree. The new version of the patch adds that. It seems like the failures on iOS are just due to the fact that the tolerance for the WPT tests is set too low. I've also marked those as failing now and will try to fix these upstream in another bug.

> > Tools/ImageDiff/ImageDiff.cpp:183
> > +                float difference = roundf((*result).percentageDifference * 100.0f) / 100.0f;
> 
> I suggest using std::round here instead of roundf. Both are the same
> function with a different name.

That makes sense. This code is a bit old, so it probably needs to be updated quite a bit. I've made this change.

> 
> > Tools/ImageDiff/ImageDiff.cpp:184
> >                  difference = std::max<float>(difference, 0.01f); // round to 2 decimal places
> 
> No need for the <float> here.

Fixed.

> 
> > Tools/ImageDiff/PlatformImage.h:61
> > +        float percentageDifference;
> > +        unsigned long pixelsDifferent;
> > +        unsigned long maxChannelDifference;
> 
> Consider initializing these all to zero here?

That makes sense to me. I've added initializers for these.

> 
> > Tools/TestRunnerShared/ReftestFunctions.h:28
> > +#include <wtf/text/WTFString.h>
> 
> Only Forward.h is needed to compile a function with String as a return value.
> 
> > Tools/TestRunnerShared/ReftestFunctions.h:36
> > +String getFuzzyMatchingTolerance(JSGlobalContextRef);
> 
> WebKit coding style says we don’t name functions like this with the word
> "get". Is String really the type we want to use for strings here?

Oh, that's right. I've fixed the name of this function and agree that maybe WTFString isn't the best type to use here. I've switched to using std::string, which should also work well.

[Martin Robinson]

Comment on attachment 442007 [details]
Patch

This patch is being superseded by work done for smfr and gsnedders.