[WebGPU] WebGPUCommandBuffer prototype
Created attachment 354864 [details] Patch
Patch doesn't apply because it relies on 191383, which will land soon.
Comment on attachment 354864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354864&action=review > Source/WebCore/ChangeLog:8 > + Begin implementation of WebGPUCommandBuffers as well as GPUQueues (MTLCommandBuffer, MTLCommandQueue). This patch has a bit more than just that. Also, I suggest you get into the habit of writing more detail in the section below, file by file. Look at one of Darin's patches for an example of how helpful that is. There isn't a good reason to simply list the set of files changed without explaining what happened. > Source/WebCore/Modules/webgpu/GPUCommandBuffer.h:2 > +/* > + * Copyright (C) 2018 Apple Inc. All rights reserved. Not for this patch, but I think all the GPU* files should be under platform, not modules. The WebGPU* files can live here. > Source/WebCore/Modules/webgpu/GPUDevice.h:31 > +#include "GPUCommandBuffer.h" > +#include "GPUQueue.h" Can't we just forward declare these? > Source/WebCore/Modules/webgpu/GPUQueue.h:47 > + PlatformQueue *platformQueue() const { return m_platformQueue.get(); } Nit: * after PlatformQueue > Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:46 > + UNUSED_PARAM(m_commandBuffer); Why do you need this? Is it because the member variable is never used? > Source/WebCore/Modules/webgpu/cocoa/GPUCommandBufferMetal.mm:57 > + buffer = [mtlQueue commandBuffer]; Don't you need to adoptNS on this? > Source/WebCore/Modules/webgpu/cocoa/GPUQueueMetal.mm:50 > + queue = [device.platformDevice() newCommandQueue]; Needs adoptNS > LayoutTests/webgpu/command-buffers.html:38 > +<script id="library" type="x-shader/x-metal"> > + #include <metal_stdlib> > + > + using namespace metal; > + > + struct Vertex > + { > + float4 position [[position]]; > + }; > + > + vertex Vertex vertex_main(uint vid [[vertex_id]]) > + { > + Vertex v; > + switch (vid) { > + case 0: > + v.position = float4(-.75, -.75, 0, 1); > + break; > + case 1: > + v.position = float4(.75, -.75, 0, 1); > + break; > + case 2: > + v.position = float4(0, .75, 0, 1); > + break; > + default: > + v.position = float4(0, 0, 0, 1); > + } > + return v; > + } > + > + fragment float4 fragment_main(Vertex vertexIn [[stage_in]]) > + { > + return float4(1.0, 0.0, 0.0, 1.0); > + } > +</script> You don't need a shader for this test. > LayoutTests/webgpu/command-buffers.html:40 > +'use strict'; Unimportant, but this isn't necessary here. > LayoutTests/webgpu/webgpu-basics.html:91 > +function render() { How is this different from the test above? Shouldn't it have a more accurate name for the moment?
(In reply to Dean Jackson from comment #3) > Comment on attachment 354864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354864&action=review > > > Source/WebCore/Modules/webgpu/GPUDevice.h:31 > > +#include "GPUCommandBuffer.h" > > +#include "GPUQueue.h" > > Can't we just forward declare these? Hmm, the only other place I can put them and have my code still compile (problems in Ref.h template code) is WebGPUDevice.h, and that makes even less sense since there is no mention of either class.
Created attachment 354970 [details] Patch
(In reply to Dean Jackson from comment #3) > Comment on attachment 354864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354864&action=review > > > Source/WebCore/ChangeLog:8 > > + Begin implementation of WebGPUCommandBuffers as well as GPUQueues (MTLCommandBuffer, MTLCommandQueue). > > This patch has a bit more than just that. Hmm, I can't really come up with a better description for this. Suggestions? There's a lot of code but the fact is, implementing this stuff is quite a bit of boilerplate.
Comment on attachment 354970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354970&action=review > Source/WebCore/Modules/webgpu/cocoa/GPUQueueMetal.mm:50 > + queue = adoptNS([device.platformDevice() newCommandQueue]); Why did you use retainPtr in the other file and adoptNS here?
Comment on attachment 354970 [details] Patch Clearing flags on attachment: 354970 Committed r238245: <https://trac.webkit.org/changeset/238245>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46105786>