[webkit-reviews] review granted: [Bug 208560] [GPU Process] Implement CanvasRenderingContext2D.getImageData() : [Attachment 392659] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 208560: [GPU Process] Implement CanvasRenderingContext2D.getImageData()
https://bugs.webkit.org/show_bug.cgi?id=208560

Attachment 392659: Patch

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




--- Comment #10 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 392659
  --> https://bugs.webkit.org/attachment.cgi?id=392659
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferMessageHandlerProxy.h:51
> +    virtual RefPtr<WebCore::ImageData>
performGetImageData(WebCore::AlphaPremultiplication outputFormat,
WebCore::IntRect srcRect) = 0;

const WebCore::IntRect&

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:60
> +    RefPtr<WebCore::ImageData>
performGetImageData(WebCore::AlphaPremultiplication outputFormat,
WebCore::IntRect srcRect) override
> +    {
> +	   return BaseConcreteImageBuffer::getImageData(outputFormat, srcRect);
> +    }

You could have the same name getImageData() instead of performGetImageData().
Just define RemoteImageBufferMessageHandlerProxy::getImageData() exactly the
same as ImageBuffer::getImageData() is defined. Change the above one to be 

    RefPtr<WebCore::ImageData> getImageData(WebCore::AlphaPremultiplication
outputFormat, const WebCore::IntRect& srcRect) const override
    {
	return BaseConcreteImageBuffer::getImageData(outputFormat, srcRect);
    }

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.cpp:108
> +	       completionHandler(WebCore::IntSize(),
IPC::SharedBufferDataReference());

Should not we do a better job here? Can't we return an ImageData with the
required size with zero bytes?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackendProxy.messages.in:29
> +    CreateImageBuffer(WebCore::FloatSize logicalSize, WebCore::RenderingMode
renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace,
WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    ReleaseImageBuffer(WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    FlushImageBufferDrawingContext(WebCore::DisplayList::DisplayList
displayList, WebKit::ImageBufferFlushIdentifier flushIdentifier,
WebKit::ImageBufferIdentifier imageBufferIdentifier)
> +    GetImageData(enum:uint8_t WebCore::AlphaPremultiplication outputFormat,
WebCore::IntRect srcRect, WebKit::ImageBufferIdentifier imageBufferIdentifier)
-> (WebCore::IntSize size, IPC::SharedBufferDataReference data) Synchronous

Is there a reason for removing the void? I am asking because Tim added asked me
to add them verbally and I did not ask why.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBuffer.h:89
> +    bool asynchronouslyFlushDrawingContext()

asynchronously is a little bit confusing since the message is asynchronous
message anyway. Can't we keep this function flushDrawingContext() and name the
other flushDrawingContextAndWait()?

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferMessageHandler.cpp:77
> +    return ImageData::create(size,
Uint8ClampedArray::create(reinterpret_cast<const uint8_t*>(data.data()),
data.size()));

The ImageData should be encoded and decoded as one unit. right?