[Webkit-unassigned] [Bug 191383] New: [WebGPU] Code quality concerns raised for 191291: [WebGPU] Experimental prototype for WebGPURenderPipeline and WebGPUSwapChain

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=191383

            Bug ID: 191383
           Summary: [WebGPU] Code quality concerns raised for 191291:
                    [WebGPU] Experimental prototype for
                    WebGPURenderPipeline and WebGPUSwapChain
           Product: WebKit
           Version: Other
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Enhancement
          Priority: P2
         Component: WebGPU
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: justin_fan at apple.com

Address code quality concerns for WebGPU after hello-triangle prototype is ready.

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

> Source/WebCore/Modules/webgpu/GPUPipelineDescriptorBase.h:37
> +    Vector<GPUPipelineStageDescriptor> stages;


Are you sure this is the right internal design? We need to differentiate between the vertex shader and the fragment shader when we compile the state. That shouldn't have to iterate through a list of things just to find which item is for the fragment shader.

> Source/WebCore/Modules/webgpu/GPUPipelineStageDescriptor.h:37
> +    const GPUShaderModule& module;


What happens when the module gets garbage collected? Will this be dangling?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:37
> +struct GPURenderPipelineDescriptor : GPUPipelineDescriptorBase {


Why struct?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:52
> +    int primitiveTopology;


This shouldn't be an int.

> Source/WebCore/Modules/webgpu/WebGPUPipelineStageDescriptor.h:39
> +    const WebGPUShaderModule* module = nullptr;


Ditto about the garbage collector

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:36
> +    enum {


enum class

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:39
> +        VERTEX = 0,
> +        FRAGMENT = 1,
> +        COMPUTE = 2


Can we use different internal names for these? Our style is not to use ALL CAPS.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:50
> +#else
> +using PlatformRenderPipeline = void;
> +using PlatformRenderPipelineSmartPtr = RefPtr<void>;
> +#endif


I think it's fine to assume we're not using metal for now. We can break that assumption one day when the linux ports want WebGPU.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:88
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;


We should be deliberate about where we use these.

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:182
> +    macro(WebGPURenderPipeline) \
> +    macro(WebGPUShaderStage) \


Why aren't the other objects in here?

> LayoutTests/webgpu/render-pipelines.html:5
> +<script id="library" type="x-shader/x-metal">


😐

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181107/eecb37cf/attachment.html>