Bug 239748

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].