[webkit-reviews] review granted: [Bug 233179] [WebGPU] Start preparing for serializing commands to the GPU process : [Attachment 444358] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 233179: [WebGPU] Start preparing for serializing commands to the GPU
process
https://bugs.webkit.org/show_bug.cgi?id=233179

Attachment 444358: Patch

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




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 444358
  --> https://bugs.webkit.org/attachment.cgi?id=444358
Patch

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

r=me with many nits.  Comments about namespaces and std::function apply several
times in the patch.

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupDescriptor.h:34
> +namespace WebKit {
> +namespace WebGPU {

namespace WebKit::WebGPU

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupEntry.h:32
> +#include <utility>
> +#include <variant>

I don't see these used directly

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupEntry.h:37
> +enum class BindingResourceType {

: uint8_t

> Source/WebKit/Shared/WebGPU/WebGPUConvertFromBackingContext.h:68
> +    virtual const PAL::WebGPU::Adapter*
convertAdapterFromBacking(WebGPUIdentifier) = 0;

Do we want to return raw pointers here?  What will be the lifetime of these
objects?

> Source/WebKit/Shared/WebGPU/WebGPUDepthStencilState.h:40
> +    bool depthWriteEnabled;

Do we want to have default initializers for these to avoid uninitialized
memory?

> Source/WebKit/Shared/WebGPU/WebGPUDeviceDescriptor.h:40
> +    // Vector<KeyValuePair<String, uint64_t>> requiredLimits;

Can we avoid adding commented code like this?

> Source/WebKit/Shared/WebGPU/WebGPUOrigin3D.h:43
> +}

You have comments elsewhere indicating that this closes a namespace.

> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteAdapterProxy.h:62
> +    Deque<std::function<void(Ref<PAL::WebGPU::Device>)>> m_callbacks;

Can we use WTF::Function instead of std::function?