[webkit-reviews] review denied: [Bug 185272] [WebGL] WEBGL_compressed_texture_astc support : [Attachment 339503] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has denied Justin Fan
<justin_fan at apple.com>'s request for review:
Bug 185272: [WebGL] WEBGL_compressed_texture_astc support
https://bugs.webkit.org/show_bug.cgi?id=185272

Attachment 339503: Patch

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




--- Comment #10 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 339503
  --> https://bugs.webkit.org/attachment.cgi?id=339503
Patch

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

I think you forgot to include the test.

> Source/WebCore/ChangeLog:10
> +	   Test: fast/canvas/webgl/webgl-compressed-texture-astc.html

This patch doesn't seem to include the test.

> Source/WebCore/html/canvas/WebGLCompressedTextureASTC.cpp:81
> +	       ||
context.graphicsContext3D()->getExtensions().supports(ASCIILiteral {
"GL_KHR_texture_compression_astc_ldr" }));

Shouldn't we do this in the constructor and convert it to a bitfield?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5429
> +    case Extensions3D::COMPRESSED_RGBA_ASTC_4x4_KHR:
> +    case Extensions3D::COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR:
> +	   {
> +	       const int kBlockSize = 16;
> +	       const int kBlockWidth = 4;
> +	       const int kBlockHeight = 4;
> +	       bytesRequired = ((width + kBlockWidth - 1) / kBlockWidth) *
((height + kBlockHeight - 1) / kBlockHeight) * kBlockSize;
> +	   }
> +	   break;

Can't we do this using a lookup table?