[webkit-reviews] review granted: [Bug 238251] [WebGPU] Implement Device::createTexture() according to the spec : [Attachment 455484] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 238251: [WebGPU] Implement Device::createTexture() according to the spec
https://bugs.webkit.org/show_bug.cgi?id=238251

Attachment 455484: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 455484
  --> https://bugs.webkit.org/attachment.cgi?id=455484
Patch

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

> Source/WebGPU/WebGPU/Texture.h:60
>      const id<MTLTexture> m_texture { nil };

Why is it safe to have a raw pointer here? Doesn’t this need to be a RetainPtr?

> Source/WebGPU/WebGPU/Texture.h:62
> +    const WGPUTextureDescriptor m_descriptor { }; // "The
GPUTextureDescriptor describing this texture."

I don’t think you need { } here. Unless WGPUTextureDescriptor is a scalar.
Otherwise the descriptor should have default values for its members rather than
a { } here.

> Source/WebGPU/WebGPU/Texture.mm:36
> +static std::optional<WGPUFeatureName>
featureRequirementForFormat(WGPUTextureFormat format)

If WGPUTextureFormat an enum? If not, why not? If it’s an enum, maybe we want
switches without defaults below so we can be reminded by compiler warnings to
revisit them all any time we add a new enumeration value.

> Source/WebGPU/WebGPU/Texture.mm:495
> +    // "descriptor.mipLevelCount must be greater than zero."
> +    if (!descriptor.mipLevelCount)
> +	   return false;

I’m not sure that this style where we quote every line of the specification
(which may no longer match this in future as it is revised) and have code doing
exactly that just below. Also, why not use "> 0" if the text version says
"greater than zero"?

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:631
> +    EXPECT_EQ(WTF::fastLog2(1u), 0u);
> +    EXPECT_EQ(WTF::fastLog2(2u), 1u);
> +    EXPECT_EQ(WTF::fastLog2(3u), 2u);
> +    EXPECT_EQ(WTF::fastLog2(4u), 2u);
> +    EXPECT_EQ(WTF::fastLog2(5u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(6u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(7u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(8u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(9u), 4u);

Is testing 1-9 sufficient coverage? Typically I like to test edge cases, like
the highest supported value, the first unsupported, and various boundaries,
etc. etc. It seems peculiar to test such a small range so close to 0.