[webkit-reviews] review granted: [Bug 191084] [WebGPU] Experimental prototype for MSL shaders and render pipelines : [Attachment 353547] 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 191084: [WebGPU] Experimental prototype for MSL shaders and render
pipelines
https://bugs.webkit.org/show_bug.cgi?id=191084

Attachment 353547: Patch

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




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

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

> Source/WTF/ChangeLog:3
> +	   [WebGPU] Experimental prototype for MSL shaders and render pipelines

This title is now wrong.

> Source/WebCore/CMakeLists.txt:-456
> -    Modules/webgpu/WebGPUSwapChainDescriptor.idl

Did this file never exist? I don't understand why it is being removed in this
patch.

> Source/WebCore/Modules/webgpu/GPUDevice.h:44
> +using PlatformDevice =
> +#if USE(METAL)
> +    MTLDevice;
> +#else
> +    void;
> +#endif

Our convention is that the entire statement goes in the #if section. So

#if USE(METAL)
using PlatformDevice = MTLDevice;
...

> Source/WebCore/Modules/webgpu/GPUDevice.h:54
> +    Ref<GPUShaderModule> createShaderModule(GPUShaderModuleDescriptor&&)
const;

Can this ever fail? I think it can if the descriptor is bogus, so it might have
to be a RefPtr.

> Source/WebCore/Modules/webgpu/GPUDevice.h:61
> +    RetainPtr<PlatformDevice> m_platformDevice;

I guess the issue here is that it is only a RetainPtr in USE(METAL) (where it
is an ObjC object). Maybe guard this with #if USE(METAL) as well for now.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:49
> +using PlatformShaderModule =
> +#if USE(METAL)
> +    MTLLibrary;
> +#else
> +    void;
> +#endif

Same as above. Although you can probably put the OBJC_PROTOCOL bit in here too.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:60
> +    RetainPtr<PlatformShaderModule> m_platformShaderModule;

And ditto with the RetainPtr.

> Source/WebCore/Modules/webgpu/cocoa/GPUDeviceMetal.mm:57
> +Ref<GPUDevice> GPUDevice::create()
> +{
> +    return adoptRef(*new GPUDevice());
> +}

We put this first in the .cpp file.

> Source/WebCore/Modules/webgpu/cocoa/GPUShaderModuleMetal.mm:57
> +Ref<GPUShaderModule> GPUShaderModule::create(const GPUDevice& device,
GPUShaderModuleDescriptor&& descriptor)
> +{
> +    return adoptRef(*new GPUShaderModule(device, WTFMove(descriptor)));
> +}

Same here. Goes first.

Also, I think we need to do something if there is no device - return nullptr.
At the moment you create what looks like a valid GPUShaderModule.

> LayoutTests/ChangeLog:8
> +	   * webgpu/webgpu-basics.html: Added.

This should be called shadermodule-basics.html or something like that.

> LayoutTests/webgpu/webgpu-basics.html:72
> +    window.webgpu.requestAdapter().then(adapter_ => {
> +	   if (!adapter_) {
> +	       testFailed("Could not create default WebGPUAdapter!")
> +	       return;
> +	   }
> +	   adapter = adapter_;
> +
> +	   defaultDevice = adapter.createDevice();
> +	   if (!defaultDevice) {
> +	       testFailed("Could not create WebGPUDevice!");
> +	       return;
> +	   }
> +
> +	   setUpShaders();
> +    }, error => {
> +	   console.log(error);
> +    });

This code is fine, but I tend to find async/await a bit easier to read.

The function would become async function setUpContexts()

And then the body would be:

adapter = await window.webgpu.requestAdapter();
if (!adapter)...

If you still think there might be an error, you put it in try catch.

> LayoutTests/webgpu/webgpu-basics.html:82
> +	   return;

This return isn't needed.