[webkit-reviews] review granted: [Bug 193457] (WIP) [WebGPU] WebGPUProgrammablePassEncoder::setBindGroup prototype : [Attachment 359510] 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 193457: (WIP) [WebGPU] WebGPUProgrammablePassEncoder::setBindGroup
prototype
https://bugs.webkit.org/show_bug.cgi?id=193457

Attachment 359510: Patch

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




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

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

> Source/WebCore/ChangeLog:11
> +	   Support for compute pipelines to follow shortly. 
> +	   Texture and sampler resources to follow after their requisite
interfaces are implemented. 

I wouldn't worry about adding these comments.

> Source/WebCore/ChangeLog:25
> +	   * platform/graphics/gpu/GPUBindGroupLayout.h: Added
ArgumentEncoderBuffer struct to retain ptr tot both MTLArgumentEncoders and
their argument MTLBuffers.

Typo tot

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17048
> +		1CA0C2F621EEDAD200A11860 /* AST */ = {
> +			isa = PBXGroup;
> +			children = (
> +				1C840B9021EC30F900D0500D /* WHLSLAddressSpace.h
*/,
> +				C21BF72521CD89E200227979 /*
WHLSLArrayReferenceType.h */,
> +				C21BF70921CD89CA00227979 /* WHLSLArrayType.h
*/,
> +				C21BF73021CD89ED00227979 /*
WHLSLAssignmentExpression.h */,
> +				C21BF70A21CD89CB00227979 /*
WHLSLBaseFunctionAttribute.h */,
> +				C21BF6FA21CD89BE00227979 /* WHLSLBaseSemantic.h
*/,
> +				C21BF71E21CD89DC00227979 /* WHLSLBlock.h */,
> +				C21BF6F621CD89B700227979 /*
WHLSLBooleanLiteral.h */,
> +				C21BF71A21CD89D800227979 /* WHLSLBreak.h */,
> +				C2138A1321DDECD300F516BA /*
WHLSLBuiltInSemantic.cpp */,
> +				C21BF72221CD89DF00227979 /*
WHLSLBuiltInSemantic.h */,
> +				C21BF71621CD89D500227979 /*
WHLSLCallExpression.h */,

What is this change?

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:38
>  OBJC_PROTOCOL(MTLArgumentEncoder);
> +OBJC_PROTOCOL(MTLBuffer);

This should probably be inside #if USE(METAL) too

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:55
> +	   bool operator!() const { return !encoder || !buffer; }

I think this would be better as an isValid() function rather than an operator.

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:64
> +    GPUBindGroupLayout(BindingsMapType&&, ArgumentEncoderBuffer&&,
ArgumentEncoderBuffer&&, ArgumentEncoderBuffer&&);

You should probably give these parameters names here. The order isn't obvious.

> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63
> +    virtual void useResource(MTLResource *, unsigned long) = 0;
> +
> +    // Render command encoder methods.
> +    virtual void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long)
{ }
> +    virtual void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned
long) { }

MTLFoo are ObjC types, so should be MTLFoo* not MTLFoo *.

> Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:69
> +    void useResource(MTLResource *, unsigned long) final;
> +    void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) final;
> +    void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) final;

Ditto.