[webkit-reviews] review granted: [Bug 211660] Adapt LocalCurrentGraphicsContext for iOS : [Attachment 398915] 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 211660: Adapt LocalCurrentGraphicsContext for iOS
https://bugs.webkit.org/show_bug.cgi?id=211660

Attachment 398915: Patch v1

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 398915
  --> https://bugs.webkit.org/attachment.cgi?id=398915
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.