rdar://92287000
Created attachment 458327 [details] Patch
Comment on attachment 458327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458327&action=review looks good! A couple Qs > Source/WebKit/Platform/cocoa/CocoaImage.mm:-56 > - if (CFStringGetLength(preferredMIMEType)) { should we keep this check or add an assert that the CFStringRef isn't empty for future callers? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:11034 > + auto [data, type] = WebKit::imageDataForCroppedImageResult(result, (__bridge CFStringRef)sourceMIMEType.get()); Do we no longer need to worry about if the result is nil in this case?
Comment on attachment 458327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458327&action=review Thanks for the review! >> Source/WebKit/Platform/cocoa/CocoaImage.mm:-56 >> - if (CFStringGetLength(preferredMIMEType)) { > > should we keep this check or add an assert that the CFStringRef isn't empty for future callers? Good call! Will add a `ASSERT(CFStringGetLength(preferredMIMEType));` here. >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:11034 >> + auto [data, type] = WebKit::imageDataForCroppedImageResult(result, (__bridge CFStringRef)sourceMIMEType.get()); > > Do we no longer need to worry about if the result is nil in this case? Good catch! I originally fell back to the original image in the case where the "markup image conversion" failed, but this is no longer desired behavior. That said, in the case where `result` is null, we should be bailing from this codepath altogether — I'll add an early return for the `!result` case above.
Created attachment 458373 [details] Patch for landing
Committed r293436 (249994@main): <https://commits.webkit.org/249994@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458373 [details].