[webkit-reviews] review granted: [Bug 75925] Implement WebGLShaderPrecisionFormat : [Attachment 135353] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 75925: Implement WebGLShaderPrecisionFormat
https://bugs.webkit.org/show_bug.cgi?id=75925

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

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


Looks good overall. A few minor issues which you can fix upon landing, or feel
free to upload a new version. r=me

> Source/Platform/chromium/public/WebGraphicsContext3D.h:289
> +    virtual void getShaderPrecisionFormat(WGC3Denum shadertype, WGC3Denum
precisiontype, WGC3Dint* range, WGC3Dint* precision) = 0;

Has the Chromium implementation of this already landed?

> Source/WebCore/html/canvas/WebGLShaderPrecisionFormat.h:38
> +    static PassRefPtr<WebGLShaderPrecisionFormat> create(GC3Dint rangeMin,
GC3Dint rangeMax, GC3Dint precision)

Please consider putting these inlined methods into a .cpp file. Having them in
the header increases code size.

>
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1217
> +void GraphicsContext3D::getShaderPrecisionFormat(GC3Denum shaderType,
GC3Denum precisionType, GC3Dint* range, GC3Dint* precision)

Architecturally speaking I think this should go in GraphicsContext3DOpenGL.cpp.
That file targets desktop GL specifically, while the Common file is used on
both ES and GL systems. This hand-coded implementation is only needed on
desktop GL, since on ES 2.0 this call actually exists.

>
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1229
> +	   range[0] = -31;

Could you add a comment indicating that these constants came from the Chromium
port, and that we believe they originally came from making the actual API call
on a representative desktop system?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:1375
> +	   range[0] = -31;

Same comment needed here. Also I think a FIXME should be added indicating that
this should be retargeted at the real getShaderPrecisionFormat call on true ES
2.0 hardware. (I believe the Qt port aims to run on ES 2.0.)