[webkit-reviews] review granted: [Bug 57248] Occlusion issues with WebGL in Safari : [Attachment 87400] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 57248: Occlusion issues with WebGL in Safari
https://bugs.webkit.org/show_bug.cgi?id=57248

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87400&action=review

Looks good except for one nit.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:57
> +    Extensions3D* extensions = getExtensions();

getExtensions() should really return a const pointer to reflect the ownership
model. Maybe just make it a const here and open a new bug to change the call
signature.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:62
> +	       m_attrs.depth = true;

Do you really need to set the depth attr to true? You can still use
depth_stencil and just waste the 24 bits of depth if you leave this false. I'm
thinking about the different behavior you might get if one implementation has
this true and another has it false, since this is an author visible flag. But I
suppose that's an issue for the spec, so this is fine for now.