Bug 221748

Created attachment 419971 [details] Patch

Comment on attachment 419971 [details] 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:1178 > + 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/RemoteGraphicsContextGLProxyFunctionsGenerated.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/RemoteGraphicsContextGLProxyFunctionsGenerated.cpp:2449 > + auto sendResult = sendSync(Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData(), Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData::Reply(returnValue)); 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 > - > + :)

Thanks for the review! (In reply to Myles C. Maxfield from comment #2) > 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? This patch doesn't really have anything to do with getImageData(), namely RemoteRenderingBackend::getImageData(). That one is used for similar purpose as the paintRenderingResultsToImageData(), but for canvas. The only reason this patch mentions it, is that the return value is the same, and the return type was redundantly complexities for that. In this patch, I remove that complexity. This patch is mostly about paintRenderingResultsToImageData. > > Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxyFunctionsGenerated.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. So this generation case is a bit different: 1) 99.9% of compiles the files don't change 2) Very, very non-trivial to review the the generator changes without reviewing the changes induced by the generator changes 3) It's not a general purpose generator, rather it generates the one and only thing it is built for As an example of 2), this patch also checks in generated code related to the IPC generator (messages.py). The stuff in "WebKit/Platform/IPC/tests" are the examples that the reviewer uses to review the stuff that gets added to messages.py. > > > Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxyFunctionsGenerated.cpp:2449 > > + auto sendResult = sendSync(Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData(), Messages::RemoteGraphicsContextGL::PaintRenderingResultsToImageData::Reply(returnValue)); > > What's the difference between paintRenderingResultsToImageData() and > getImageData()? Why do we need both? They're totally different functions for different classes. (The real reason is that WebGL does not use ImageBuffer as its abstraction for drawing buffer / display buffer, where as canvas uses it for drawing buffer and there's not similar distinction between drawing buffer and display buffer for canvas.) > > 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. Sure, sync vs async discussion it's not part of this patch. (Not to mention that the GetImageData itself is not a major part of this patch.)

Committed r272786: <https://commits.webkit.org/r272786> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419971 [details].

<rdar://problem/74281552>