Bug 210974 – Use CocoaImage platform abstraction for NSImage/UIImage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

Use CocoaImage platform abstraction for NSImage/UIImage.

Following on from comments in Bug 210814, move CocoaImage into its own header for reuse in other classes.

There aren't as many opportunities for this as you might think, though:

$ grep -l NSImage `grep -l -r UIImage Source | grep -v ChangeLog | grep -v .order` 
Source/WebKitLegacy/mac/WebView/WebView.mm
Source/WebKitLegacy/mac/WebView/WebViewData.h
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebView.h
Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.h
Source/WebKit/UIProcess/QuickLookThumbnailLoader.h
Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm
Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.h
Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm
Source/WebCore/editing/cocoa/HTMLConverter.mm

The uses in WebCore and WebKitLegacy aren't really cross-platform, so all of the interesting use cases are in WebKit.

However, the initial version of the patch doesn't use CocoaImage in public headers as I wasn't sure if we were okay with doing things like this in Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:

-#if TARGET_OS_IPHONE
-- (void)takeSnapshotWithConfiguration:(nullable WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(UIImage * _Nulla
ble snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(ios(11.0));
-#else
-- (void)takeSnapshotWithConfiguration:(nullable WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macos(10.13));
-#endif

+- (void)takeSnapshotWithConfiguration:(nullable WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(CocoaImage * _Nullable snapshotImage, NSError * _Nullable error))completionHandler WK_API_AVAILABLE(macos(10.13), ios(11.0));

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #0)
> However, the initial version of the patch doesn't use CocoaImage in public
> headers

Yes, I don’t think it is good to use it in public headers.

[David Kilzer (:ddkilzer)]

Created attachment 397471 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 397471 [details]
Patch v1

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:983
> -#if PLATFORM(MAC)
> -- (void)takeSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void(^)(NSImage *, NSError *))completionHandler
> +- (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?

[Darin Adler]

Comment on attachment 397471 [details]
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.

[Darin Adler]

Comment on attachment 397471 [details]
Patch v1

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

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

Oops, not sure why I left that comment in. Please don’t change this! I am surprised but we should take no risk on this at this time, just based on that surprise!

[David Kilzer (:ddkilzer)]

Comment on attachment 397471 [details]
Patch v1

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

>> Source/WebKit/Platform/cocoa/CocoaImage.h:26
>> +#import <wtf/PlatformUse.h>
> 
> This should not be necessary. The prefix header takes care of this.

Will remove.

>> Source/WebKit/Platform/cocoa/CocoaImage.h:35
>> +#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.

Only used in Objective-C++ for now, so will just keep using statement.

>> Source/WebKit/Platform/cocoa/CocoaImage.h:44
>> +#endif
> 
> Ditto.

Only used in Objective-C++ for now, so will just keep using statement.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1017
>> +        handler(image.get(), nil);
> 
> I don’t think this change is needed. Nice to use auto, but no need to use CocoaImage here.

Will keep the variable name change and auto and revert the class name change.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1032
>> +            handler(image.get(), error.get());
> 
> I don’t think this change is needed. Nicer local variable name, but no need to use CocoaImage here.

Will keep the variable name change and revert the class name change.

>>> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:154
>>> +        return [[_cocoaImage copy] autorelease];
>> 
>> Surprised that we need copy/autorelease.
> 
> Oops, not sure why I left that comment in. Please don’t change this! I am surprised but we should take no risk on this at this time, just based on that surprise!

Okay.

>> Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:163
>> +#endif
> 
> I would have used the class names NSImage and UIImage here, not CocoaImage, but it’s OK either way I suppose.

Will revert the class name change.

>> Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:77
>>  #endif
> 
> Here too.

Will revert the class name change.

[David Kilzer (:ddkilzer)]

Created attachment 397480 [details]
Patch v2 (land safely after EWS)

[EWS]

Committed r260667: <https://trac.webkit.org/changeset/260667>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397480 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62337926>