[webkit-reviews] review granted: [Bug 238428] [WebGPU] Implement CommandEncoder::copyBufferToTexture() according to the spec : [Attachment 455954] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 238428: [WebGPU] Implement CommandEncoder::copyBufferToTexture() according
to the spec
https://bugs.webkit.org/show_bug.cgi?id=238428

Attachment 455954: Patch

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




--- Comment #6 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 455954
  --> https://bugs.webkit.org/attachment.cgi?id=455954
Patch

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

with non-ascii chars fixed
with integral auto variables declared explicitly as their intended type.

> Source/WebGPU/WebGPU/Texture.mm:2370
> +    auto blockWidth =
Texture::texelBlockWidth(fromAPI(imageCopyTexture.texture).descriptor().format)
;

explicitly declare the intended type

> Source/WebGPU/WebGPU/Texture.mm:2556
> +    if ((imageCopyTexture.origin.x + copySize.width) >
subresourceSize.width)

I think generally a+b>c is written without parenthesis. Even though the spec
has the parenthesis, maybe the code would still be nicer without.

> Source/WebGPU/WebGPU/Texture.mm:2585
> +    auto blockWidth = Texture::texelBlockWidth(format);

all these calculations are important part of the validation.
the integral types should be declared explicitly to ensure less mistakes

> Source/WebGPU/WebGPU/Texture.mm:2618
> +	   if (layout.bytesPerRow <= bytesInLastRow)

off-by-one
should be
  if (layout.bytesPerRow < bytesInLastRow)
      return false;

> Source/WebGPU/WebGPU/Texture.mm:2630
> +    auto requiredBytesInCopy = 0;

this is declared as int, but it does calculations with layout members that are
uint64_t.
Instead, declare it explicitly as uint64_t