Bug 233358 – [WebGPU] Implement GPU process's message receivers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WebGPU] Implement GPU process's message receivers

[Myles C. Maxfield]

Created attachment 444800 [details]
Patch

[Dean Jackson]

Comment on attachment 444800 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp:52
> +    // FIXME: Implement this.

ASSERT_NOT_IMPLEMENTED()?

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:72
> +    auto computePassEncoder = m_backing->beginComputePass(*convertedDescriptor);

What happens if convertedDescriptor still doesn't have a value at this point?

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:88
> +    auto convertedSource = m_objectRegistry.convertBufferFromBacking(source);
> +    ASSERT(convertedSource);
> +    auto convertedDestination = m_objectRegistry.convertBufferFromBacking(source);
> +    ASSERT(convertedDestination);
> +    m_backing->copyBufferToBuffer(*convertedSource, sourceOffset, *convertedDestination, destinationOffset, size);

Should this (and the other similar code) actually test that the converted buffers exist rather than just ASSERT?

[Myles C. Maxfield]

Comment on attachment 444800 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:72
>> +    auto computePassEncoder = m_backing->beginComputePass(*convertedDescriptor);
> 
> What happens if convertedDescriptor still doesn't have a value at this point?

RemoteComputePassEncoder::create takes an optional. If the input was nullopt, we should be passing nullopt to the consumer, but if the input wasn't nullopt, we should verify that it could be converted successfully.

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:88
>> +    m_backing->copyBufferToBuffer(*convertedSource, sourceOffset, *convertedDestination, destinationOffset, size);
> 
> Should this (and the other similar code) actually test that the converted buffers exist rather than just ASSERT?

oh, you mean only call the backing function if the lookup was successful? I'll update the patch to do that.

[Myles C. Maxfield]

Created attachment 444858 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 444800 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp:52
>> +    // FIXME: Implement this.
> 
> ASSERT_NOT_IMPLEMENTED()?

It looks like doing this forces adding [[ noreturn ]] to the function signature in the header, which I think is pollution. I'll just implement this ASAP.

[Myles C. Maxfield]

Created attachment 444862 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 444867 [details]
Patch for committing

[EWS]

Committed r286087 (244474@main): <https://commits.webkit.org/244474@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444867 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85627098>