[webkit-reviews] review granted: [Bug 221748] GPU process WebGL context toDataURL does not work for non-premultiplied contexts : [Attachment 419971] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 221748: GPU process WebGL context toDataURL does not work for
non-premultiplied contexts
https://bugs.webkit.org/show_bug.cgi?id=221748

Attachment 419971: Patch

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




--- Comment #2 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 419971
  --> https://bugs.webkit.org/attachment.cgi?id=419971
Patch

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

> Source/WebKit/ChangeLog:15
> +	   This does not make any failing test pass, as the tests test also
premultiplied
> +	   case, which needs other currently unimplemented code.

:(

>
Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLFunctionsGenerated.h:1
178
> +	   returnValue = m_context->paintRenderingResultsToImageData();

I see this is the handler for paintRenderingResultsToImageData(), and I see the
web process saying sendSync(PaintRenderingResultsToImageData) below, but it
looks like the web process always says sendSync(GetImageData), and I don't see
this patch adding a handler for getImageData() in the GPU process. Am I missing
something? Is it autogenerated?

> Source/WebKit/Platform/IPC/ImageDataReference.h:-33
> -class ImageDataReference {

:)

>
Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxyFunctionsGene
rated.cpp:2445
> +RefPtr<WebCore::ImageData>
RemoteGraphicsContextGLProxy::paintRenderingResultsToImageData()

Why is this checked into a file called *Generated.cpp? I'd expect generated
files not to be checked in.

>
Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxyFunctionsGene
rated.cpp:2449
> +	   auto sendResult =
sendSync(Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData(),
Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData::Reply(retu
rnValue));

What's the difference between paintRenderingResultsToImageData() and
getImageData()? Why do we need both?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:150
> +    sendSync(Messages::RemoteRenderingBackend::GetImageData(outputFormat,
srcRect, renderingResourceIdentifier),
Messages::RemoteRenderingBackend::GetImageData::Reply(imageData),
m_renderingBackendIdentifier, 1_s);

so sad we need a synchronous message here. I think it's unavoidable though.

> Tools/Scripts/generate-gpup-webgl:252
> -    
> +

:)