Store image resolution in layout test snapshots, and have ImageDiff read it
Created attachment 445369 [details] Patch
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.
(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.
Created attachment 445372 [details] Patch
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].
<rdar://problem/85852852>
(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.
Very happy what you landed, of course. Glad you liked my suggestion of just using double.
I originally followed the code in mayHaveDensityCorrectedSize().