[webkit-reviews] review granted: [Bug 195077] [Web GPU] Buffer updates part 2: setSubData, GPU/CPU synchronization : [Attachment 363047] 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 195077: [Web GPU] Buffer updates part 2: setSubData, GPU/CPU
synchronization
https://bugs.webkit.org/show_bug.cgi?id=195077

Attachment 363047: Patch

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




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

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

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:90
> +	   arrayBuffer ? promise.resolve(*arrayBuffer) : promise.reject();

I suggest a real if statement here. I prefer ternaries only when assigning
values.

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:120
> +    static const auto s_readOnlyMask = GPUBufferUsage::Index |
GPUBufferUsage::Vertex | GPUBufferUsage::Uniform | GPUBufferUsage::TransferSrc;

Why do we need this as a static in the header now?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:66
> +    // FIXME: Metal best practices: Read-only one-time-use data less than 4
KB should not allocate a MTLBuffer and be used in [MTLCommandEncoder set*Bytes]
calls instead.

Nice.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:118
> +    if (offset + data.byteLength() > m_byteLength) {

We need to use checked arithmetic here. It can be in a followup patch, but
please add a FIXME.

Look for Checked<> stuff in WebGL or GraphicsContext3D as examples.