Implement the WebGL 2 Query objects. /* Query Objects */ WebGLQuery? createQuery(); void deleteQuery(WebGLQuery? query); [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? query); void beginQuery(GLenum target, WebGLQuery? query); void endQuery(GLenum target); /* TODO: document return type */ any getQuery(GLenum target, GLenum pname); /* TODO: document return type */ any getQueryParameter(WebGLQuery? query, GLenum pname);
<rdar://problem/15002395>
<rdar://problem/28228150>
Created attachment 404781 [details] Patch
Comment on attachment 404781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review Looking good overall James - couple of comments. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879 > + if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query)) Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920 > if (!addResult.isNewEntry) { It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by: - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN) - Attempt to start query B of the same type (leads to INVALID_OPERATION) - Attempt to end query A This will yield INVALID_OPERATION when it shouldn't. Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?
Comment on attachment 404781 [details] Patch Attempting to remove duplicate comments.
Created attachment 404890 [details] review feedback
Comment on attachment 404781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879 >> + if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query)) > > Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests? Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920 >> if (!addResult.isNewEntry) { > > It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by: > - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN) > - Attempt to start query B of the same type (leads to INVALID_OPERATION) > - Attempt to end query A > > This will yield INVALID_OPERATION when it shouldn't. > > Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests? There are tests for this. It turns out WTF::HashMap::add does not overwrite.
Comment on attachment 404781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review >>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879 >>> + if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query)) >> >> Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests? > > Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests. Possible to quickly add a test to sdk/tests/conformance2/query/query.html ? It looks like this isn't checked at least by that test. >>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920 >>> if (!addResult.isNewEntry) { >> >> It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by: >> - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN) >> - Attempt to start query B of the same type (leads to INVALID_OPERATION) >> - Attempt to end query A >> >> This will yield INVALID_OPERATION when it shouldn't. >> >> Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests? > > There are tests for this. It turns out WTF::HashMap::add does not overwrite. Ah right, I misread the documentation for HashMap::add. Thanks for checking.
Comment on attachment 404890 [details] review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=404890&action=review Looks good to me! > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1890 > + if (isContextLostOrPending() || !query || !query->object() || !validateWebGLObject("isQuery", query)) This will generate INVALID_VALUE if it receives a query from a different context, where Chromium will just return false. Not sure what the better behavior is.
Created attachment 404998 [details] rebase
Created attachment 405002 [details] rebaseline layout test expando-loss-2.html
Created attachment 405100 [details] mark reviewed
Committed r264807: <https://trac.webkit.org/changeset/264807> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405100 [details].