and its query
Created attachment 135256 [details] Patch
Ken, please review. Developing in Webkit becomes more and more painful when new files are added.
Created attachment 135353 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 135353 [details] Patch 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.)
Comment on attachment 135353 [details] Patch Attachment 135353 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12321246
Created attachment 135382 [details] Patch
(In reply to comment #5) > (From update of attachment 135353 [details]) > 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? Yes. > > > 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. Done. > > > 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. Done, and also add the function to GraphicsContext3DOpenGLES.cpp > > > 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? Done. > > > 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.) Done.
Comment on attachment 135382 [details] Patch Attachment 135382 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12322293
Comment on attachment 135382 [details] Patch Thanks for the updates. Looks good. r=me again, though looks like there are still build issues.
(In reply to comment #10) > (From update of attachment 135382 [details]) > Thanks for the updates. Looks good. r=me again, though looks like there are still build issues. The build issue in chromium was due to FakeWebGraphicsContext3D missing the implementation of getShaderPrecisionFormat, which I fixed in the patch. Hopefully the bot will turn green.
Committed r113092: <http://trac.webkit.org/changeset/113092>