[webkit-reviews] review granted: [Bug 217558] [GPU Process][Resource caching 3/7]: Introduce RemoteResourceCacheProxy to manage the remote resources in Web Process : [Attachment 411829] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 217558: [GPU Process][Resource caching 3/7]: Introduce
RemoteResourceCacheProxy to manage the remote resources in Web Process
https://bugs.webkit.org/show_bug.cgi?id=217558

Attachment 411829: Patch

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




--- Comment #11 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 411829
  --> https://bugs.webkit.org/attachment.cgi?id=411829
Patch

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

Renaming can be done in later patches.

> Source/WebCore/platform/graphics/ImageBuffer.h:65
> +    virtual RemoteResourceIdentifier remoteResourceIdentifier() const {
return { }; }

Maybe RemoteResourceIdentifier should be RenderingResourceIdentifier so it
doesn't look so out of place here.

> Source/WebKit/WebProcess/GPU/graphics/PlatformRemoteImageBufferProxy.h:47
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebKit::UnacceleratedRemoteImageBufferProxy)
> +    static bool isType(const WebCore::ImageBuffer& imageBuffer) { return
imageBuffer.remoteResourceIdentifier() && !imageBuffer.isAccelerated(); }
> +SPECIALIZE_TYPE_TRAITS_END()
> +
> +#if HAVE(IOSURFACE)
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebKit::AcceleratedRemoteImageBufferProxy)
> +    static bool isType(const WebCore::ImageBuffer& imageBuffer) { return
imageBuffer.remoteResourceIdentifier() && imageBuffer.isAccelerated(); }
> +SPECIALIZE_TYPE_TRAITS_END()

This makes me nervous. Future refactoring could easily make isAccelerated() a
dynamic thing on an ImageBuffer. Type traits need to reliably detect which
concrete class an object is, not ask questions about its behavior.

I would suggest adding isUnacceleratedRemoteImageBufferProxy(),
isAcceleratedRemoteImageBufferProxy() etc.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:69
> +    void commitFlushDisplayList(DisplayListFlushIdentifier flushIdentifier)

Is this a "didCommitFlush" that comes from the GPUP?

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:98
> +	      
const_cast<RemoteImageBufferProxy&>(*this).flushDisplayList(displayList);

I think we should remove the "flush" terminology for sending bits of display
lists. There are really only 2 things we need (with preliminary names):

send(displayList)
waitForFlush()

I also think we should avoid using "commit" in this context.
Also, maybe the flush identifier can just be a special kind of display list
item that acts like a marker in the stream.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:130
> +	   static constexpr unsigned maxWaitingFlush = 3;

We have to get rid of this timeout. If the wait goes too long, we should just
kill the GPU Process.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:137
> +    void flushDisplayList(const WebCore::DisplayList::DisplayList&
displayList) override

Why isn't this just send()?

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

1s timeout is bad. What do we do if it hits?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:148
> +    if (imageBuffer->isAccelerated())
> +	  
downcast<AcceleratedRemoteImageBufferProxy>(*imageBuffer).createBackend(logical
Size, backendSize, resolutionScale, colorSpace, WTFMove(handle));
> +    else
> +	  
downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).createBackend(logic
alSize, backendSize, resolutionScale, colorSpace, WTFMove(handle));

Why is this not just one call to a virtual function?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:160
> +    if (imageBuffer->isAccelerated())
> +	  
downcast<AcceleratedRemoteImageBufferProxy>(*imageBuffer).commitFlushDisplayLis
t(flushIdentifier);
> +    else
> +	  
downcast<UnacceleratedRemoteImageBufferProxy>(*imageBuffer).commitFlushDisplayL
ist(flushIdentifier);

Why is this not just one call to a virtual function?