Bug 197032 – [Web GPU] Implement API default values

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

[Justin Fan]

[Web GPU] Implement API default values

[Justin Fan]

Created attachment 367668 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 367668 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add default values and 'required' qualifiers recently merged to the WebGPU API.

It's pretty frightening that this patch doesn't update almost any .cpp or .mm files, which means we were happily accepting garbage values before. I would have hoped that before this patch, all the C++ code that received these values would have done more default value creation or exception/error throwing, and this patch could have deleted all that C++ code in favor of using the IDL to represent the same checks.

> Source/WebCore/Modules/webgpu/WebGPUBufferBinding.h:37
> +    uint64_t offset = 0;

Shouldn't this be done in the IDL?

> Source/WebCore/Modules/webgpu/WebGPURenderPipelineDescriptor.h:41
> +    Optional<WebGPUPipelineStageDescriptor> fragmentStage;

👍

> Source/WebCore/platform/graphics/gpu/GPUBlendDescriptor.h:59
> +    GPUBlendFactor srcFactor { GPUBlendFactor::One };
> +    GPUBlendFactor dstFactor { GPUBlendFactor::Zero };
> +    GPUBlendOperation operation { GPUBlendOperation::Add };

Doesn't the IDL take care of this for you? And ditto below.

> LayoutTests/ChangeLog:24
> +        * webgpu/bind-groups.html:
> +        * webgpu/blend-triangle-strip.html:
> +        * webgpu/blit-commands.html:
> +        * webgpu/buffer-command-buffer-races.html:
> +        * webgpu/buffer-resource-triangles.html:
> +        * webgpu/color-write-mask-triangle-strip.html:
> +        * webgpu/compute-squares.html:
> +        * webgpu/depth-enabled-triangle-strip.html:
> +        * webgpu/draw-indexed-triangles.html:
> +        * webgpu/js/webgpu-functions.js:
> +        (createBasicDepthTexture):
> +        * webgpu/texture-triangle-strip.html:
> +        * webgpu/textures-textureviews.html:
> +        * webgpu/vertex-buffer-triangle-strip.html:
> +        * webgpu/whlsl.html:

Just deleting code from existing tests isn't very exhaustive. There should probably be a single new file that has a bunch of tests in it, where each test specifies every value except one, and then draws something, and we determine from the rendered content (or from some other mechanism) whether the default value was specified.

If we made such a test we should consider contributing it to the WG. The WG is going to try to make a comprehensive test suite.

[Justin Fan]

Created attachment 367761 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 367761 [details]
Patch for landing

Rejecting attachment 367761 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 367761, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/11919784

[Justin Fan]

Created attachment 367763 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 367763 [details]
Patch for landing

Clearing flags on attachment: 367763

Committed r244442: <https://trac.webkit.org/changeset/244442>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50031486>