Provide return types for custom WebGLRenderingContext methods
Created attachment 171493 [details] Patch
Comment on attachment 171493 [details] Patch Attachment 171493 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14626678
Comment on attachment 171493 [details] Patch Attachment 171493 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14557069
Created attachment 171498 [details] Patch
Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken? Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch...
(In reply to comment #5) > Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken? Maybe we should have reviewbot add a warning for any WebGL code changes until we've incorporated the Khronos tests into WebKit? Something like what Ken says above.
(In reply to comment #5) > Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken? > > Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch... No, Ken, thanks a lot for pointing out the suite, I'll run it. Regarding no code changes: I should have been more explicit---no changes in v8 bindings. Overall, if I run the suite under Chromium with this patch in, will it be enough? What is the best approach to test that patch against remaining platforms?
Comment on attachment 171498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.idl:533 > // object getExtension(in DOMString name); Please remove the comment. > Source/WebCore/html/canvas/WebGLRenderingContext.idl:554 > // DOMString[] getSupportedExtensions() ditto
(In reply to comment #7) > What is the best approach to test that patch against remaining platforms? There isn't any EWS, so you'd have to try on the platforms manually if you can. The latest Khronos test suite has a nice tool to run the tests and gather a report. I guess if you don't have access to the platforms, you'll just have to land confidently :)
Comment on attachment 171498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.idl:555 > + [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions(); Do you really mean DOMString? I would have expected this to return sequence<DOMString>
Comment on attachment 171498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review >> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555 >> + [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions(); > > Do you really mean DOMString? I would have expected this to return sequence<DOMString> Let me try again: Do you really mean DOMString[]? I would have expected this to return sequence<DOMString>
(In reply to comment #7) > (In reply to comment #5) > > Did you run the WebGL conformance suite at https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html to ensure nothing was broken? > > > > Clearly the autogenerated code changes somewhat, as otherwise the Qt and Qt-WK2 bots wouldn't have been broken with the first version of the patch... > > No, Ken, thanks a lot for pointing out the suite, I'll run it. > > Regarding no code changes: I should have been more explicit---no changes in v8 bindings. Overall, if I run the suite under Chromium with this patch in, will it be enough? Ideally you'd run it on Mac OS with Safari as well to test the JSC bindings. However, the EWS bots run some subset of the WebGL tests so you will probably be OK. > What is the best approach to test that patch against remaining platforms? I generally test WebGL patches on Mac OS with both Chromium and Safari, and rely on the EWS to catch build failures, etc. affecting other ports.
(In reply to comment #11) > (From update of attachment 171498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review > > >> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555 > >> + [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions(); > > > > Do you really mean DOMString? I would have expected this to return sequence<DOMString> > > Let me try again: > > Do you really mean DOMString[]? I would have expected this to return sequence<DOMString> Right, Adam's correct; see http://www.khronos.org/registry/webgl/specs/latest/ .
Created attachment 173930 [details] Patch
Sorry for late responses. Two test of the suite fail: conformance/textures/texture-mips.html (17 of 19 passed) failed: texture that is only using the smallest 2 mips should draw with green failed: texture that is only using smallest mips should draw with cyan but those tests fail w/o the patch. Regarding comments and sequence. I tried to keep the simplest IDL close to the latest spec, hence keeping comments if current decl differs from the spec and not using sequence<DOMString> which might be not supported by other platforms like object with which I started. I am uploading new patch with sequence<DOMString> to see if it passes EWS. (In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 171498 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171498&action=review > > > > >> Source/WebCore/html/canvas/WebGLRenderingContext.idl:555 > > >> + [StrictTypeChecking, Custom] DOMString[] getSupportedExtensions(); > > > > > > Do you really mean DOMString? I would have expected this to return sequence<DOMString> > > > > Let me try again: > > > > Do you really mean DOMString[]? I would have expected this to return sequence<DOMString> > > Right, Adam's correct; see http://www.khronos.org/registry/webgl/specs/latest/ .
Comment on attachment 173930 [details] Patch Nice, thanks. r=me. Please wait for this to pass the EWS before cq+'ing.
Everything looks green, cq'ing while the repo is quite. Many thanks to all for review and comments. (In reply to comment #16) > (From update of attachment 173930 [details]) > Nice, thanks. r=me. Please wait for this to pass the EWS before cq+'ing.
Comment on attachment 173930 [details] Patch Clearing flags on attachment: 173930 Committed r134580: <http://trac.webkit.org/changeset/134580>
All reviewed patches have been landed. Closing bug.