[webkit-reviews] review granted: [Bug 218529] [GPU Process] Use the Ref counting of ImageBuffer to control its life cycle in Web Process and GPU Process : [Attachment 413391] 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 218529: [GPU Process] Use the Ref counting of ImageBuffer to control its
life cycle in Web Process and GPU Process
https://bugs.webkit.org/show_bug.cgi?id=218529

Attachment 413391: Patch

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




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

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

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191
> +    m_displayList.cacheImageBuffer(makeRef(imageBuffer));

Rather than this, you should the work in willAppendItemOfType(), because there
are going to be other drawing commands that use ImageBuffers (e.g.
clipToImageBuffer() which you did not fix in this patch).

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:54
> +    if (item.type() == ItemType::DrawImageBuffer) {
> +	   auto& drawItem = static_cast<DrawImageBuffer&>(item);
> +	   if (auto* imageBuffer =
m_imageBuffers.get(drawItem.renderingResourceIdentifier()))
> +	       drawItem.apply(m_context, *imageBuffer);
> +	   return;
> +    }

Also needs to deal with clipToImageBuffer().

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:57
> +    if (m_delegate && m_delegate->apply(item, m_context))
> +	   return;

It's unclear how someone reading this code would know whether it's correct for
this to be after the ImageBuffer special-casing.