[webkit-reviews] review denied: [Bug 110936] [WebGL] Support for texImage2D of type HALF_FLOAT_OES with ImageData, HTMLImageElement, HTMLCanvasElement and HTMLVideoElement. : [Attachment 195479] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Nayan Kumar K
<nayankk at motorola.com>'s request for review:
Bug 110936: [WebGL] Support for texImage2D of type HALF_FLOAT_OES with
ImageData, HTMLImageElement, HTMLCanvasElement and HTMLVideoElement.
https://bugs.webkit.org/show_bug.cgi?id=110936

Attachment 195479: Patch
https://bugs.webkit.org/attachment.cgi?id=195479&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195479&action=review


Given today's announcement regarding the Chromium project, I'm not sure I
should review this patch; dino should. However, I'm positive that this patch
isn't correct as written, so setting r- to make that clear. (It's good work so
far, but needs to be made robust. Negative tests are also necessary given the
incomplete state.)

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-3753
> -

The rest of this patch only has incomplete support for HALF_FLOAT_OES packing
and unpacking. If you are going to remove this check, then you need to modify
validateTexFuncFormatAndType so that it only supports the combination of
RGBA/HALF_FLOAT_OES when dealing with non-ArrayBufferView sources, and
generates INVALID_OPERATION otherwise.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:141
> +	       ASSERT_NOT_REACHED();

This is missing support for the other formats like RGB, ALPHA, LUMINANCE, and
LUMINANCE_ALPHA.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1083
> +}

This is missing support for the other formats like RGB, ALPHA, LUMINANCE, and
LUMINANCE_ALPHA.