[webkit-reviews] review granted: [Bug 217596] [GPU Process][Resource caching 6/7]: Cache the NativeImage in GPU Process and allow referencing it with its RemoteResourceIdentifier : [Attachment 413847] 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 217596: [GPU Process][Resource caching 6/7]: Cache the NativeImage in GPU
Process and allow referencing it with its RemoteResourceIdentifier
https://bugs.webkit.org/show_bug.cgi?id=217596

Attachment 413847: Patch

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




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

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

> Source/WebCore/platform/graphics/NativeImage.h:61
> +    void setRenderingResourceIdentifier(RenderingResourceIdentifier
renderingResourceIdentifier) { m_renderingResourceIdentifier =
renderingResourceIdentifier; }

It's a bit weird to have m_renderingResourceIdentifier be initialized to the
generated value, then set here. Should RenderingResourceIdentifier be passed
into the constructor as an Optional<>?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1300
> +    RenderingResourceIdentifier m_renderingResourceIdentifier;

We can all this m_imageIdentifier here

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:201
> +    if (!m_delegate)
> +	   m_displayList.cacheNativeImage(image);
> +    else
> +	   m_delegate->cacheNativeImage(image);

This is a bit confusing. It assumes knowledge of delegate behavior.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:65
>	   if (!m_remoteRenderingBackendProxy)
>	       return;
>	   flushDrawingContext();
> +	  
m_remoteRenderingBackendProxy->remoteResourceCacheProxy().releaseImageBuffer(m_
renderingResourceIdentifier);
>	  
m_remoteRenderingBackendProxy->releaseRemoteResource(m_renderingResourceIdentif
ier);

Please move to the .cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:89
>      {
>	   ASSERT(m_remoteRenderingBackendProxy);
> +	  
m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheImageBuffer(*thi
s);
> +
>	   m_drawingContext.displayList().setItemBufferClient(this);
>	   m_drawingContext.displayList().setTracksDrawingItemExtents(false);
>      }

Move to the cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:191
> +    void cacheNativeImage(WebCore::NativeImage& image) override
> +    {
> +	   if (m_remoteRenderingBackendProxy)
> +	      
m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheNativeImage(imag
e);
> +    }

Put in the cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:54
> +    using ImageBufferHashMap = HashMap<WebCore::RenderingResourceIdentifier,
WebCore::ImageBuffer*>;
> +    using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier,
WebCore::NativeImage*>;

Can we use WeakPtr here?