[webkit-reviews] review granted: [Bug 195347] [Web GPU] Enable drawing GPUTextures and GPUTextureViews in the render pipeline, and related GPUBindGroup updates : [Attachment 363717] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 195347: [Web GPU] Enable drawing GPUTextures and GPUTextureViews in the
render pipeline, and related GPUBindGroup updates
https://bugs.webkit.org/show_bug.cgi?id=195347

Attachment 363717: Patch

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




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 363717
  --> https://bugs.webkit.org/attachment.cgi?id=363717
Patch

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

> Source/WebCore/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: webgpu/texture-triangle-strip.html

It would be nice if you could provide a description of what this patch actually
does.

> Source/WebCore/Modules/webgpu/GPUBindGroupLayoutBinding.h:31
> +#include <wtf/OptionSet.h>

I don't think you need this.

> Source/WebCore/Modules/webgpu/GPUBindGroupLayoutBinding.h:46
> +    GPUShaderStageFlags visibility;

Shouldn't this be GPUShaderStageBit::Flags?

> Source/WebCore/Modules/webgpu/WebGPURenderPassDescriptor.cpp:61
> +	   if (!colorAttachment.attachment
> +	       || !colorAttachment.attachment->texture()
> +	       || !colorAttachment.attachment->texture()->isOutputAttachment())
{

This could all go on one line.

> Source/WebCore/Modules/webgpu/WebGPUTexture.cpp:68
> +    if (!m_texture)
> +	   LOG(WebGPU, "GPUTexture::destroy(): Invalid operation!");
> +    else
> +	   destroyImpl();

Not sure this will compile in Release configuration.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:80
> +    endBlitEncoding();

I would prefer to put the conditional in here, and an ASSERT in the
endBlitEncoding.

if (m_blitEncoder)
  endBlitEncoding();

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:91
> +    if (!m_blitEncoder)
> +	   return;

And here would just be ASSERT(m_blitEncoder), unless you call it from other
places.