Bug 233609 – Store image resolution in layout test snapshots, and have ImageDiff read it

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Fraser (smfr)]

Store image resolution in layout test snapshots, and have ImageDiff read it

[Simon Fraser (smfr)]

Created attachment 445369 [details]
Patch

[Darin Adler]

Comment on attachment 445369 [details]
Patch

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

> Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:67
> +    float resolutionWidth = 72.0 * scaleFactor;
> +    float resolutionHeight = 72.0 * scaleFactor;

Why convert double to float here instead of just using kCFNumberDoubleType?

> Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:69
> +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));

Seems incorrect, but likely harmless, to use kCFNumberFloat32Type here instead of kCFNumberFloatType.

Better to just use doubles anyway, though.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:80
> +    auto properties = CGImageSourceCopyPropertiesAtIndex(imageSource, 0, nullptr);
> +    if (properties) {

Could scope the variable to the if.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:85
> +            if (CFNumberGetValue(resolutionXProperty, kCFNumberFloat32Type, &resolutionX) && CFNumberGetValue(resolutionYProperty, kCFNumberFloat32Type, &resolutionY)) {

kCFNumberFloatType is correct here, but also, could use double

> Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:120
> +    float resolutionWidth = 72.0 * imageSize.width / windowSize.width;
> +    float resolutionHeight = 72.0 * imageSize.height / windowSize.height;
> +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));

Same comments as about about double and about kCFNumberFloatType.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #2)
> Comment on attachment 445369 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445369&action=review
> 
> > Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:67
> > +    float resolutionWidth = 72.0 * scaleFactor;
> > +    float resolutionHeight = 72.0 * scaleFactor;
> 
> Why convert double to float here instead of just using kCFNumberDoubleType?
> 
> > Tools/DumpRenderTree/cg/PixelDumpSupportCG.cpp:69
> > +    auto resolutionWidthNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionWidth));
> > +    auto resolutionHeightNumber = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberFloat32Type, &resolutionHeight));
> 
> Seems incorrect, but likely harmless, to use kCFNumberFloat32Type here
> instead of kCFNumberFloatType.

Looking at the CF source, it seems that kCFNumberFloatType and kCFNumberFloat32Type are synonymous, as are kCFNumberFloat64Type and kCFNumberFloatDoubleType.

[Simon Fraser (smfr)]

Created attachment 445372 [details]
Patch

[EWS]

Committed r286285 (244644@main): <https://commits.webkit.org/244644@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445372 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85852852>

[Darin Adler]

(In reply to Simon Fraser (smfr) from comment #3)
> Looking at the CF source, it seems that kCFNumberFloatType and
> kCFNumberFloat32Type are synonymous, as are kCFNumberFloat64Type and
> kCFNumberFloatDoubleType.

Yes, understood. That’s what I meant when I said the mistake was "harmless".

But my comment is: why use the wrong one? One is intended for the "C float type", and that is kCFNumberFloatType.

[Darin Adler]

Very happy what you landed, of course. Glad you liked my suggestion of just using double.

[Simon Fraser (smfr)]

I originally followed the code in mayHaveDensityCorrectedSize().