[webkit-reviews] review granted: [Bug 221811] WebGL contexts do not work as source for Context2D drawImage calls in GPU process : [Attachment 420312] rebase

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 221811: WebGL contexts do not work as source for Context2D drawImage calls
in GPU process
https://bugs.webkit.org/show_bug.cgi?id=221811

Attachment 420312: rebase

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 420312
  --> https://bugs.webkit.org/attachment.cgi?id=420312
rebase

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

I’m not a deep subject matter expert, but this looks good.

> Source/WebCore/platform/graphics/cairo/GraphicsContextGLCairo.cpp:109
> +void GraphicsContextGLOpenGL::paintToCanvas(const
GraphicsContextGLAttributes& attributes, Ref<ImageData>&& imageData, const
IntSize& canvasSize, GraphicsContext& context)

Some ambiguity since both contextAttributes() and this attributes argument
exist. Is there any way to make the distinction between the two clearer. I’m
guessing that it’s about source attributes vs. destination attributes?

> Source/WebCore/platform/graphics/cg/GraphicsContextGLCG.cpp:518
> +    auto attrs = attributes;

No reason to copy these just to check two flags a couple lines later. I suggest
getting rid of this local variable.

>
Source/WebCore/platform/graphics/cocoa/GraphicsContextGLIOSurfaceSwapChain.cpp:
55
> +    void* result = m_displayBuffer.handle;
> +    m_displayBuffer.handle = nullptr;
> +    return result;

Might be nice to write this with std::exchange:

    return std::exchange(m_displayBuffer.handle, nullptr);

>
Source/WebCore/platform/graphics/cocoa/GraphicsContextGLIOSurfaceSwapChain.cpp:
61
> +    m_spareBuffer = WTFMove(m_displayBuffer);
> +    m_displayBuffer = WTFMove(buffer);

Might be nice to write this with std::exchange:

    m_spareBuffer = std::exchange(m_displayBuffer, WTFMove(buffer));

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:55
> +    return std::unique_ptr<RemoteGraphicsContextGL>(new
RemoteGraphicsContextGLCocoa(attributes, gpuConnectionToWebProcess,
graphicsContextGLIdentifier, renderingBackend));

Can this be written using make_unique instead? It’s worth a little effort to
avoid the bare "new" for cases like this.

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:68
> +    auto* surface = m_swapChain.displayBuffer().surface.get();
> +    if (surface)

Nicer to define inside the if.

    if (auto surface = ...