[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 bugzilla-daemon at webkit.org
Mon Feb 15 09:50:58 PST 2021

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

Attachment 420312: rebase


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

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.

> +    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);

> +    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 = ...

More information about the webkit-reviews mailing list