[webkit-reviews] review granted: [Bug 205823] [WebGL 2] Implement transform feedback and pass transform feedback conformance tests : [Attachment 387307] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 10 12:37:08 PST 2020


Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 205823: [WebGL 2] Implement transform feedback and pass transform feedback
conformance tests
https://bugs.webkit.org/show_bug.cgi?id=205823

Attachment 387307: Patch

https://bugs.webkit.org/attachment.cgi?id=387307&action=review




--- Comment #6 from Dean Jackson <dino at apple.com> ---
Comment on attachment 387307
  --> https://bugs.webkit.org/attachment.cgi?id=387307
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387307&action=review

You're going to have to rebase, sorry.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:105
> +    // Remove all references to WebGLObjects so if they are the last
reference
> +    // they will be freed before the last context is removed from the
context group.

I don't think we need this comment.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:331
> -    memcpy(static_cast<char*>(dstData->baseAddress()) +
dstData->byteOffset() + dstOffset * elementSize, ptr, copyLength *
elementSize);
> +    if (ptr)
> +	   memcpy(static_cast<char*>(dstData->baseAddress()) +
dstData->byteOffset() + dstOffset * elementSize, ptr, copyLength *
elementSize);
> +

If ptr is null, wouldn't we want to throw an error?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1100
> +    auto addResult = m_activeQueries.add(targetKey, makeRefPtr(&query));
> +
> +    if (!addResult.isNewEntry) {

You can do this in one line - no need for addResult.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1268
> +	       synthesizeGLError(GraphicsContextGL::INVALID_OPERATION,
"bindTransformFeedback", "cannot bind a deleted Transform Feedback object");

Nit: Transform Feedback should probably be one word.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2541
> +    size_t index = m_boundTransformFeedbackBuffers.find(buffer);
> +    if (index < m_boundTransformFeedbackBuffers.size())
> +	   m_boundTransformFeedbackBuffers[index] = nullptr;

Why can't you call m_boundTransformFeedbackBuffers.removeAll(buffer) ?

> Source/WebCore/platform/graphics/GraphicsContextGL.h:1068
> +    // WebGL2 Transform Feedback calls
> +    virtual PlatformGLObject createTransformFeedback() = 0;
> +    virtual void deleteTransformFeedback(PlatformGLObject) = 0;
> +    virtual GCGLboolean isTransformFeedback(PlatformGLObject) = 0;
> +    virtual void bindTransformFeedback(GCGLenum, PlatformGLObject) = 0;
> +    virtual void beginTransformFeedback(GCGLenum) = 0;
> +    virtual void endTransformFeedback() = 0;
> +    virtual void transformFeedbackVaryings(PlatformGLObject, const
Vector<String>&, GCGLenum) = 0;
> +    virtual void getTransformFeedbackVarying(PlatformGLObject, GCGLuint,
ActiveInfo&) = 0;
> +
> +    virtual void bindBufferBase(GCGLenum, GCGLuint, PlatformGLObject) = 0;
> +

I think I landed all these just moments ago.


More information about the webkit-reviews mailing list