Bug 193341 – [WebGPU] WebGPUBindGroup and device::createBindGroup prototype

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Fan]

[WebGPU] WebGPUBindGroup and device::createBindGroup prototype

[Justin Fan]

Created attachment 358840 [details]
Patch

[Justin Fan]

Created attachment 358852 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 358852 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUBindGroup.cpp:33
> +Ref<WebGPUBindGroup> WebGPUBindGroup::create(RefPtr<GPUBindGroup>&& bindGroup)

Why not use Ref<>? You won't have to perform as many null checks.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:118
> +        return WebGPUBindGroup::create(nullptr);

This should refuse to create the object. In JS, if you call this function with garbage, it should return null, not a real object with nothing inside it.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:125
> +        auto bindingResource = WTF::visit(BindingResourceVisitor(), binding.resource);

Please use WTF::makeVisitor()

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:130
> +            return WebGPUBindGroup::create(nullptr);

Ditto from above

[Dean Jackson]

Comment on attachment 358840 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:112
> +struct BindingResourceVisitor {
> +    Optional<GPUBindingResource> operator()(RefPtr<WebGPUTextureView> textureView) const
> +    {
> +        if (textureView)
> +            return static_cast<GPUBindingResource>(textureView->texture());
> +        return WTF::nullopt;
> +    }
> +
> +    Optional<GPUBindingResource> operator()(WebGPUBufferBinding binding) const
> +    {
> +        // FIXME: What kind of validation should be performed on binding.offset and binding.size?
> +        if (binding.buffer)
> +            return static_cast<GPUBindingResource>(GPUBufferBinding { binding.buffer->buffer(), binding.offset, binding.size });
> +        return WTF::nullopt;
> +    }
> +};

We usually do this with WTF::makeVisitor inline, just above the call to ::visit.

Also, we tend to use inline function syntax rather than operator()... so it would be something like this:

auto visitor = WTF::makeVisitor([&](RefPtr<WebGPUTextureView> textureView) -> Optional<GPUBindingResource> {
        ...
    }, [&](WebGPUBufferBinding binding) -> Optional<GPUBindingResource> {
        ...
    });

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:124
> +    Vector<GPUBindGroupBinding> bindGroupBindings;
> +    bindGroupBindings.reserveCapacity(descriptor.bindings.size());
> +
> +    for (const auto& binding : descriptor.bindings) {

You could do this with Vector::map. e.g.

auto bindGroupBindings = descriptor.bindings.map([] (const auto& binding) { .... } );

However, if you want to handle errors... I guess you'd have to have a variable outside the scope of the function.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:125
> +        auto bindingResource = WTF::visit(BindingResourceVisitor(), binding.resource);

See WebGLRenderingContextBase::texSubImage2D for an example.

[Justin Fan]

Created attachment 358856 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 358856 [details]
Patch for landing

Clearing flags on attachment: 358856

Committed r239853: <https://trac.webkit.org/changeset/239853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47197555>