Bug 211660 – Adapt LocalCurrentGraphicsContext for iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Adapt LocalCurrentGraphicsContext for iOS.

The original motivation was a small clean up in UIProcess/Cocoa/WebPageProxyCocoa.mm, but this resulted in a really nice cleanup in WebProcess/WebCoreSupport/mac/WebDragClientMac.mm.

[David Kilzer (:ddkilzer)]

Created attachment 398915 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 398916 [details]
Interesting changes to WebCore/platform

[Darin Adler]

Comment on attachment 398915 [details]
Patch v1

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

> Source/WebCore/platform/cocoa/LocalCurrentGraphicsContext.h:60
> +class ContextContainer {
> +    WTF_MAKE_NONCOPYABLE(ContextContainer);
> +public:
> +    ContextContainer(GraphicsContext& graphicsContext)
> +        : m_graphicsContext(graphicsContext.platformContext())
> +    {
> +    }
> +
> +    CGContextRef context() { return m_graphicsContext; }
> +private:
> +    PlatformGraphicsContext* m_graphicsContext;
> +};

This class is not new, but it seems pointless. Should be removed. The two places that use it should just be calling the platformContext function. Also, the use of both CGContextRef and PlatformGraphicsContext* in the implementation of the class is puzzling.

> Source/WebCore/platform/ios/LocalCurrentGraphicsContextIOS.mm:58
> +CGContextRef LocalCurrentGraphicsContext::cgContext()
> +{
> +    return m_savedGraphicsContext.platformContext();
> +}

Should be inlined in the header. And not have separate versions for macOS and iOS.

> Source/WebCore/platform/mac/LocalCurrentGraphicsContextMac.mm:60
> +CGContextRef LocalCurrentGraphicsContext::cgContext()
> +{
> +    return m_savedGraphicsContext.platformContext();
> +}

Should be inlined in the header. And not have separate versions for macOS and iOS.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:56
> +using PlatformDragImage = NSImage *;

Not a big fan of the word "Platform" in the class name here, but I don’t have a better idea.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:60
>  #import "UIKitSPI.h"
> +
> +using PlatformDragImage = CGImageRef;

Not a big fan of combining the "using" and the include like this. I would do the include in a separate paragraph above.

[David Kilzer (:ddkilzer)]

Comment on attachment 398915 [details]
Patch v1

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

>> Source/WebCore/platform/cocoa/LocalCurrentGraphicsContext.h:60
>> +};
> 
> This class is not new, but it seems pointless. Should be removed. The two places that use it should just be calling the platformContext function. Also, the use of both CGContextRef and PlatformGraphicsContext* in the implementation of the class is puzzling.

Will do in a separate patch.

>> Source/WebCore/platform/ios/LocalCurrentGraphicsContextIOS.mm:58
>> +}
> 
> Should be inlined in the header. And not have separate versions for macOS and iOS.

Will fix.

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContextMac.mm:60
>> +}
> 
> Should be inlined in the header. And not have separate versions for macOS and iOS.

Will fix.

>> Source/WebKit/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:56
>> +using PlatformDragImage = NSImage *;
> 
> Not a big fan of the word "Platform" in the class name here, but I don’t have a better idea.

How about just "DragImage"?  I guess we need to be careful about defining this in unified sources, but it's probably fine for now.

We could have reused CocoaImage if the iOS code has used UIImage instead of CGImageRef.  Not sure it's worth a FIXME, though.

>> Source/WebKit/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:60
>> +using PlatformDragImage = CGImageRef;
> 
> Not a big fan of combining the "using" and the include like this. I would do the include in a separate paragraph above.

Will separate before landing.

[David Kilzer (:ddkilzer)]

Comment on attachment 398915 [details]
Patch v1

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

>>> Source/WebCore/platform/cocoa/LocalCurrentGraphicsContext.h:60
>>> +};
>> 
>> This class is not new, but it seems pointless. Should be removed. The two places that use it should just be calling the platformContext function. Also, the use of both CGContextRef and PlatformGraphicsContext* in the implementation of the class is puzzling.
> 
> Will do in a separate patch.

(Will change PlatformGraphicsContext* to CGContextRef in this patch, though.)

[David Kilzer (:ddkilzer)]

Created attachment 398940 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Created attachment 398941 [details]
Patch for landing v2

[David Kilzer (:ddkilzer)]

Comment on attachment 398940 [details]
Patch for landing

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

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:57
> -#if PLATFORM(IOS_FAMILY)
> +#if USE(APPKIT)
>  #import "UIKitSPI.h"
>  #endif

That's...not correct.

[EWS]

Committed r261442: <https://trac.webkit.org/changeset/261442>

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

[Radar WebKit Bug Importer]

<rdar://problem/63057667>