[webkit-reviews] review denied: [Bug 194665] [WebGPU] (WIP) Buffer updates part 1: async mapping functions, unmap, and destroy : [Attachment 362667] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 194665: [WebGPU] (WIP) Buffer updates part 1: async mapping functions,
unmap, and destroy
https://bugs.webkit.org/show_bug.cgi?id=194665

Attachment 362667: Patch

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




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

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

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.h:41
> +    Optional<GPUBindGroupDescriptor> validateAndConvertToGPUVersion() const;

I think this should be named asGPUBindGroupDescriptor()

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:63
> +    unmap();

Will it be an error to call unmap on an already unmapped buffer?

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:68
> +void WebGPUBuffer::rejectOrPendMappingPromise(ArrayBufferPromise&& promise,
bool isRead)

I don't like this name. /me looks around for existing examples....

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:76
> +    m_buffer->pendMappingPromise(WTFMove(promise), isRead);

I wonder if it is worth doing more testing here... probably not if GPUBuffer
does all the smart stuff (like test if it is already mapped).

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:48
> +using ArrayBufferPromise =
DOMPromiseDeferred<IDLInterface<JSC::ArrayBuffer*>>;

We can't do this. DOMPromiseDeferred lives in bindings/js, and we're inside
/platform/.

So we can't use Promises at this level. Instead, they'll be held at the WebGPU
level. Down here we'll have to implement our own callbacks.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:131
> +   
MicrotaskQueue::mainThreadQueue().append(std::make_unique<VoidMicrotask>([this,
protectedThis = makeRef(*this)] () mutable {

Similarly, microtasks are part of the DOM. We'll have to use another queue or
make our own. I wonder if Media has anything like this... it must.