[webkit-reviews] review canceled: [Bug 190101] [macOS] Update Pasteboard::read to prioritize native representations over TIFF : [Attachment 449824] For EWS

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 190101: [macOS] Update Pasteboard::read to prioritize native
representations over TIFF
https://bugs.webkit.org/show_bug.cgi?id=190101

Attachment 449824: For EWS

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




--- Comment #14 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 449824
  --> https://bugs.webkit.org/attachment.cgi?id=449824
For EWS

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

Thanks for taking a look!

>> Source/WebCore/platform/mac/PasteboardMac.mm:438
>> +static RefPtr<SharedBuffer>
imageDataAfterRemovingMetadata(Ref<SharedBuffer>&& buffer)
> 
> It seems a bit unfortunate that this very-generically-named function will
break all animated images (GIF and APNG). I think your changelog suggests they
should never get here; maybe an assertion or type check or something at the
top? Or add all of the animation properties to your allowlist?

Yes, that's right — we'll only get here for TIFF, PNG and JPEG images. In the
future, if we add support for reading image data in image/gif format or support
for animated PNG, we'll need to adjust this logic to preserve animation-related
properties as well.

As we discussed, I'll adjust this to only add the first frame of the image,
since that's the behavior we currently end up with if we have animated PNG, and
leave a FIXME to track making GIF/APNG readable in this codepath.

>> Source/WebCore/platform/mac/PasteboardMac.mm:455
>> +		if (![name isEqualToString:(__bridge NSString
*)kCGImagePropertyTIFFDictionary]) {
> 
> Not sure I follow what's going on here; you're only preserving image
orientation in the TIFF properties? What about the other formats?

So from local testing, it seems like image orientation is a subway of the tiff
image property dictionary (i.e. the dictionary labeled {TIFF} in the image
properties).

All images appear to have this {TIFF} dictionary…at least, from what I could
tell. I'll do more testing and double check this.