[webkit-reviews] review granted: [Bug 192516] [WebGPU] Implement WebGPUBuffer, and some nullibility consistency in WebGPU : [Attachment 356998] 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 192516: [WebGPU] Implement WebGPUBuffer, and some nullibility consistency
in WebGPU
https://bugs.webkit.org/show_bug.cgi?id=192516

Attachment 356998: Patch

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




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

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

> Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:38
> +RefPtr<WebGPURenderPassEncoder>
WebGPURenderPassEncoder::create(Ref<WebGPUCommandBuffer>&& creator,
Ref<GPURenderPassEncoder>&& encoder)
>  {
> -    return adoptRef(*new WebGPURenderPassEncoder(WTFMove(creator),
WTFMove(encoder)));
> +    return adoptRef(new WebGPURenderPassEncoder(WTFMove(creator),
WTFMove(encoder)));

s/creator/commandBuffer/

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:40
> +namespace JSC {
> +
> +class ArrayBuffer;
> +    
> +}

No need for blank lines.

> Source/WebCore/platform/graphics/gpu/GPUBufferUsage.h:36
> +class GPUBufferUsage : public RefCounted<GPUBufferUsage> {

This doesn't need to be RefCounted. There will never be an instance of this
class.

> Source/WebCore/platform/graphics/gpu/GPUBufferUsage.h:46
> +    static const unsigned long NONE = 0;
> +    static const unsigned long MAP_READ = 1;
> +    static const unsigned long MAP_WRITE = 2;
> +    static const unsigned long TRANSFER_SRC = 4;
> +    static const unsigned long TRANSFER_DST = 8;
> +    static const unsigned long INDEX = 16;
> +    static const unsigned long VERTEX = 32;
> +    static const unsigned long UNIFORM = 64;
> +    static const unsigned long STORAGE = 128;

I think we should follow WebKit style here and use CamelCase. As long as the
values are what the IDL exposes, we should be ok.

e.g.

enum class GPUBufferUsage : uint32_t {
  None = 0,
  MapRead = 1,
  MapWrite = 2,
  ...
}

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:48
> +    const char* const functionName = "GPUBuffer::create()";
> +#if LOG_DISABLED
> +    UNUSED_PARAM(functionName);
> +#endif

I suggest just putting this into each LOG statement rather than having a local
variable.

> LayoutTests/webgpu/buffers.html:10
> +'use strict';

Not needed for this test.

> LayoutTests/webgpu/buffers.html:14
> +    assert_true(buffer instanceof WebGPUBuffer, "creatBuffer returned a
WebGPUBuffer");

Typo: create.

> LayoutTests/webgpu/buffers.html:23
> +    let floatArray = new Float32Array(arrayBuffer);
> +    assert_equals(floatArray.length, 4);
> +
> +    floatArray[0] = -1;
> +    floatArray[1] = 1;
> +    floatArray[2] = 0;
> +    floatArray[3] = 1;

It turns out that this isn't how you'd use the API, but it's still exercising
the things you've built, so we should keep it.