[webkit-reviews] review granted: [Bug 210974] Use CocoaImage platform abstraction for NSImage/UIImage : [Attachment 397471] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 210974: Use CocoaImage platform abstraction for NSImage/UIImage
https://bugs.webkit.org/show_bug.cgi?id=210974

Attachment 397471: Patch v1

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




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

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

Nice improvement.

> Source/WebKit/Platform/cocoa/CocoaImage.h:26
> +#import <wtf/PlatformUse.h>

This should not be necessary. The prefix header takes care of this.

> Source/WebKit/Platform/cocoa/CocoaImage.h:35
> +#ifdef __cplusplus
> +using CocoaImage = NSImage;
> +#else
> +#define CocoaImage NSImage
> +#endif

If we need something that works with Objective-C (not C++) then I suggest we
use typedef, not #define, and not using. But, can this just be an Objective-C++
header? Then it would use using unconditionally.

> Source/WebKit/Platform/cocoa/CocoaImage.h:44
> +#ifdef __cplusplus
> +using CocoaImage = UIImage;
> +#else
> +#define CocoaImage UIImage
> +#endif

Ditto.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:983
>> +- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration
*)snapshotConfiguration completionHandler:(void(^)(CocoaImage *, NSError
*))completionHandler
> 
> This is the least-satisfying improvement.  Maybe I need to extract the
platform-specific code into static methods?

Can chip away at it more later.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1017
> -	   RetainPtr<NSImage> nsImage = adoptNS([[NSImage alloc]
initWithCGImage:cgImage.get() size:NSMakeSize(snapshotWidth, imageHeight)]);
> -	   handler(nsImage.get(), nil);
> +	   auto image = adoptNS([[CocoaImage alloc]
initWithCGImage:cgImage.get() size:NSMakeSize(snapshotWidth, imageHeight)]);
> +	   handler(image.get(), nil);

I don’t think this change is needed. Nice to use auto, but no need to use
CocoaImage here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1032
> -	       RetainPtr<UIImage> uiImage;
> +	       RetainPtr<CocoaImage> image;
>	       
>	       if (!snapshotImage)
>		   error = createNSError(WKErrorUnknown);
>	       else
> -		   uiImage = adoptNS([[UIImage alloc]
initWithCGImage:snapshotImage scale:deviceScale
orientation:UIImageOrientationUp]);
> +		   image = adoptNS([[CocoaImage alloc]
initWithCGImage:snapshotImage scale:deviceScale
orientation:UIImageOrientationUp]);
>	       
> -	       handler(uiImage.get(), error.get());
> +	       handler(image.get(), error.get());

I don’t think this change is needed. Nicer local variable name, but no need to
use CocoaImage here.

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:154
> +	   return [[_cocoaImage copy] autorelease];

Surprised that we need copy/autorelease.

> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:163
> +#if USE(APPKIT)
> +    _cocoaImage = adoptNS([[CocoaImage alloc]
initWithCGImage:_image->makeCGImageCopy().get()
size:NSSizeFromCGSize(_boundingRect.size)]);
> +#else
> +    _cocoaImage = adoptNS([[CocoaImage alloc]
initWithCGImage:_image->makeCGImageCopy().get()]);
> +#endif

I would have used the class names NSImage and UIImage here, not CocoaImage, but
it’s OK either way I suppose.

>
Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.
mm:77
> +#if USE(APPKIT)
> +    return [[[CocoaImage alloc]
initWithCGImage:image->bitmap().makeCGImage().get() size:NSZeroSize]
autorelease];
> +#else
> +    return [[[CocoaImage alloc]
initWithCGImage:image->bitmap().makeCGImage().get()] autorelease];
>  #endif

Here too.