[webkit-reviews] review denied: [Bug 63635] Improve WebGL object lifetime management in WebGLRenderingContext : [Attachment 99321] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 30 14:49:53 PDT 2011
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.
More information about the webkit-reviews
mailing list