[webkit-reviews] review denied: [Bug 110818] [WebGL] Support for texImage2D of type HALF_FLOAT_OES with ArrayBufferView. : [Attachment 190856] Patch

Thu Feb 28 20:42:23 PST 2013

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

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

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

Looking good. Please make one more code change and incorporate the layout test
in the new location for the WebGL tests.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3769
> +    // Uploading ImageData to half floating point texture is not supported
yet

Here and in the other places in this patch, add "FIXME:" at the start of the
comment, and also file a bug about fixing this and link to that bug. Also, move
this check down after the call to if (!validateSettableTexFormat(...)) in all
these places. That way INVALID_ENUM will still be generated (by
validateTexFuncFormatAndType) if the extension hasn't been enabled yet.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:1
> +<!DOCTYPE html>

The new location for these tests is LayoutTests/webgl/ and you can use the
script LayoutTests/webgl/generate-webgl-tests.py script to pull in the script
verbatim from the one you just contributed to Khronos. Unfortunately, the
webgl/ folder isn't yet enabled on any port, so it might be worthwhile to add
the test in this location temporarily. However, please also add it in the new
location using that script. Also, in order to get this patch landed, you may
want to mark the test as having text differences temporarily for the failing
platforms (e.g. in LayoutTests/platform/mac/TestExpectations). Don't skip the
test, just mark it as failing. Then you can get the results off the bots and
see why it's failing.