[WebGPU] WebGPUBindGroup and device::createBindGroup prototype
Created attachment 358840 [details] Patch
Created attachment 358852 [details] Patch
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
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.
Created attachment 358856 [details] Patch for landing
Comment on attachment 358856 [details] Patch for landing Clearing flags on attachment: 358856 Committed r239853: <https://trac.webkit.org/changeset/239853>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47197555>