[webkit-reviews] review denied: [Bug 63635] Improve WebGL object lifetime management in WebGLRenderingContext : [Attachment 99321] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 63635: Improve WebGL object lifetime management in WebGLRenderingContext
https://bugs.webkit.org/show_bug.cgi?id=63635

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

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

Nice work; generally looks good except for one cleanup issue in
detachAndRemoveAllObjects.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2033
> +	   m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);

This is correct, but it might be worth a note here that OpenGL ES 2.0 specifies
INVALID_ENUM in this case, while desktop GL specifies INVALID_OPERATION.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2043
> +	       return
WebGLGetInfo(PassRefPtr<WebGLTexture>(reinterpret_cast<WebGLTexture*>(object)))
;

I think you should be using adoptRef rather than calling the PassRefPtr
constructor explicitly here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2061
> +	       return
WebGLGetInfo(PassRefPtr<WebGLRenderbuffer>(reinterpret_cast<WebGLRenderbuffer*>
(object)));

Same thing about using adoptRef here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4001
>      }

This looks very complicated and flaky. If you're worried about
WebGLObject::detachContext() removing the object from the table and changing
the iteration order, copy the HashSet<WebGLObject*> into a Vector<WebGLObject*>
and iterate down the vector.

Also, you might want to consider having WebGLObject::detachContext() remove the
object from this table rather than the destructor of WebGLObject. Right now
calling detachContext() on the WebGLObject prevents the WebGLObject from
removing itself from this table.