[webkit-reviews] review denied: [Bug 75906] Add WebGLContextGroup as step toward sharing WebGL resources : [Attachment 121956] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Gregg Tavares <gman at google.com>'s
request for review:
Bug 75906: Add WebGLContextGroup as step toward sharing WebGL resources
https://bugs.webkit.org/show_bug.cgi?id=75906

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

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


Good refactoring overall. Minor comments and questions only. If all of my
comments are invalid and you think this patch should proceed as is, go ahead
and mark r? again.

> Source/WebCore/GNUmakefile.list.am:548
> +	DerivedSources/WebCore/JSWebGLContextGroup.h \

WebGLContextGroup doesn't have any IDL -- so these shouldn't be necessary (and
their presence should, I think, cause build failures).

> Source/WebCore/html/canvas/WebGLContextGroup.cpp:74
> +    if (m_contexts.isEmpty()) {

How about writing this as a pre-check: if (m_contexts.size() == 1 &&
m_contexts.contains(context)) { detachAndRemvoeAllObjects(); } thereby avoiding
the re-adding and removal of the context.

> Source/WebCore/html/canvas/WebGLContextGroup.cpp:89
> +    removeObject(object);

Why is this call necessary? The are two other similar calls in
WebGLRenderingContext.cpp, addSharedObject() and addContextObject().

> Source/WebCore/html/canvas/WebGLContextObject.h:37
> +class WebGLContextObject : public WebGLObject {

This class definitely needs a comment indicating that it represents WebGL
resources that are per-context, rather than shared among contexts.

> Source/WebCore/html/canvas/WebGLContextObject.h:67
> +#endif // WebGLObject_h

Wrong #include guard.

> Source/WebCore/html/canvas/WebGLFramebuffer.h:46
> +    void setAttachmentForBoundFramebuffer(GraphicsContext3D*, GC3Denum
attachment, GC3Denum texTarget, WebGLTexture*, GC3Dint level);

Since WebGLFramebuffer is a per-context object, it's unnecessary to pass in the
GraphicsContext3D* here, as well as in removeAttachmentFromBoundFramebuffer and
onAccess. I think the changes to those methods, and the calling code in
WebGLRenderingContext, should be reverted in order to simplify the patch.

> Source/WebCore/html/canvas/WebGLFramebuffer.h:-87
> -    bool isBound() const;

Why remove this? It seems that the associated assertions would still be
correct, since WebGLFramebuffer is a per-context object.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4186
> +    removeSharedObject(object);

Per comment above, I think this line should be removed.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4198
> +    removeContextObject(object);

Per comment above, I think this line should be removed.