[webkit-reviews] review granted: [Bug 233609] Store image resolution in layout test snapshots, and have ImageDiff read it : [Attachment 445369] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 233609: Store image resolution in layout test snapshots, and have ImageDiff
read it
https://bugs.webkit.org/show_bug.cgi?id=233609

Attachment 445369: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 445369
  --> https://bugs.webkit.org/attachment.cgi?id=445369
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.