[webkit-reviews] review granted: [Bug 233310] [WebGPU] Add converters from serializable descriptors to interface descriptors : [Attachment 444654] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 233310: [WebGPU] Add converters from serializable descriptors to interface
descriptors
https://bugs.webkit.org/show_bug.cgi?id=233310

Attachment 444654: Patch

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




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

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

Can you test any of this? Or, what is your plan for testing?

> Source/WebKit/Shared/WebGPU/WebGPUCompilationMessage.cpp:38
> +    return
PAL::WebGPU::CompilationMessage::create(String(compilationMessage.message),
compilationMessage.type, compilationMessage.lineNum,
compilationMessage.linePos, compilationMessage.offset,
compilationMessage.length);

Surprised you have to explicitly create a new String here. What is
CompilationMessage.message?

> Source/WebKit/Shared/WebGPU/WebGPUComputePassTimestampWrites.cpp:53
> +    for (const auto& backingTimestampWrite : computePassTimestampWrites) {
> +	   auto timestampWrite = convertFromBacking(backingTimestampWrite);
> +	   if (!timestampWrite)
> +	       return std::nullopt;
> +    }

Did you forget to append the timestampWrite to timestampWrites?

> Source/WebKit/Shared/WebGPU/WebGPUExternalTextureBindingLayout.cpp:38
> +    return { { } };

??

> Source/WebKit/Shared/WebGPU/WebGPURenderPassDescriptor.cpp:56
> +    if (renderPassDescriptor.depthStencilAttachment &&
!depthStencilAttachment)

I think if renderPassDescriptor.depthStencilAttachment wasn't null, then
depthStencilAttachment also won't be? Or could convertFromBacking still return
a nullopt? i.e. can you just test depthStencilAttachment?

> Source/WebKit/Shared/WebGPU/WebGPURenderPassTimestampWrites.cpp:53
> +    for (const auto& backingTimestampWrite : renderPassTimestampWrites) {
> +	   auto timestampWrite = convertFromBacking(backingTimestampWrite);
> +	   if (!timestampWrite)
> +	       return std::nullopt;
> +    }

You forgot to populate timestampWrites.

> Source/WebKit/Shared/WebGPU/WebGPURenderPipelineDescriptor.cpp:73
> +    auto fragment = ([&] () -> std::optional<PAL::WebGPU::FragmentState> {
> +	   if (renderPipelineDescriptor.fragment)
> +	       return convertFromBacking(*renderPipelineDescriptor.fragment);
> +	   return std::nullopt;
> +    })();
> +    if (renderPipelineDescriptor.fragment && !fragment)
> +	   return std::nullopt;

Same question as above - is it enough to simply test fragment? Or early return
if !rPD.fragment?

> Source/WebKit/Shared/WebGPU/WebGPUVertexState.cpp:48
> +	   if (backingBuffer) {
> +	       auto buffer = convertFromBacking(*backingBuffer);
> +	       if (!buffer)
> +		   return std::nullopt;

Missing buffers.uncheckedAppend(buffer)?

Also, since you still append in the zero-buffer case, maybe this whole loop
could be replaced with a vertexState.buffers.map()