[webkit-reviews] review granted: [Bug 217820] Support EXIF image resolution in CoreGraphics (macOS/iOS) : [Attachment 412096] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Noam Rosenthal <noam at webkit.org>'s
request for review:
Bug 217820: Support EXIF image resolution in CoreGraphics (macOS/iOS)
https://bugs.webkit.org/show_bug.cgi?id=217820

Attachment 412096: Patch

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




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 412096
  --> https://bugs.webkit.org/attachment.cgi?id=412096
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:159
> +    ImageOrientation orientation = respectOrientation ?
orientationForCurrentFrame() : ImageOrientation(ImageOrientation::None);

auto

> Source/WebCore/platform/graphics/BitmapImage.cpp:164
> +    FloatSize correctedSizeFloat(correctedSize ?
FloatSize(correctedSize.value()) : size());

auto correctedSizeFloat = correctedSize ? FloatSize(correctedSize.value()) :
size();

> Source/WebCore/platform/graphics/BitmapImage.cpp:169
> +    FloatSize srcSize = sourceSize();

Did you see my comment about src vs. source? Did you respond? I understand that
you might not agree, but was hoping to see your response.

> Source/WebCore/platform/graphics/BitmapImage.cpp:235
> +    FloatRect srcRect(requestedSrcRect);

auto srcRect = requestedSrcRect;

> Source/WebCore/platform/graphics/BitmapImage.cpp:243
>      FloatSize scaleFactorForDrawing =
context.scaleFactorForDrawing(destRect, srcRect);

auto

> Source/WebCore/platform/graphics/BitmapImage.cpp:244
> +    IntSize sizeForDrawing = expandedIntSize(srcSize *
scaleFactorForDrawing);

auto

> Source/WebCore/platform/graphics/ImageResolution.cpp:36
> +    FloatSize sizeFromResolution(sourceSize.width() * DefaultResolution /
metadata.resolution.width(), sourceSize.height() * DefaultResolution /
metadata.resolution.height());

auto sizeFromResolution = sourceSize * DefaultSize / metadata.resolution;

> Source/WebCore/platform/graphics/ImageResolution.h:35
> +    static const unsigned DefaultResolution = 72;

constexpr

> Source/WebCore/platform/graphics/ImageResolution.h:37
> +    // This range intentionally matches the resolution values from the EXIF
spec.

OK, but why?

> Source/WebCore/platform/graphics/ImageSource.cpp:571
> +    Optional<IntSize> size =
frameMetadataAtIndexCacheIfNeeded<Optional<IntSize>>(0,
(&ImageFrame::densityCorrectedSize), &m_densityCorrectedSize,
ImageFrame::Caching::Metadata);

auto

No need for parentheses around &ImageFrame::densityCorrectedSize and I don’t
think they add readability.

> Source/WebCore/platform/graphics/ImageSource.cpp:578
> +    return orientation.usesWidthAsHeight() ?
Optional<IntSize>(size.value().transposedSize()) : size;

Better to use makeOptional() rather than Optional<IntSize>().

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:213
> +    return ImageResolution::densityCorrectedSize(FloatSize(sourceWidth,
sourceHeight), {
> +	   FloatSize(preferredWidth, preferredHeight),
> +	   FloatSize(resolutionWidth, resolutionHeight),
> +	   static_cast<ImageResolution::ResolutionUnit>(resolutionUnit)
> +    });

Can probably use braces here instead of FloatSize(). So { sourceWidth,
sourceHeight } instead of FloatSize(sourceWidth, sourceHeight). I like those
better. What do you think?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:428
> +    RetainPtr<CFDictionaryRef> properties =
adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), index,
createImageSourceMetadataOptions().get()));

auto