[webkit-reviews] review granted: [Bug 30349] WebGL LayoutTests fail on buildbot : [Attachment 41983] replacement patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 30349: WebGL LayoutTests fail on buildbot
https://bugs.webkit.org/show_bug.cgi?id=30349

Attachment 41983: replacement patch
https://bugs.webkit.org/attachment.cgi?id=41983&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================

> +static void setPixelFormat(Vector<CGLPixelFormatAttribute>& attribs, int
colorBits, int depthBits, bool accelerated, bool supersample, bool closest)
> +{
> +    attribs.clear();
> +    
> +    attribs.append(kCGLPFAColorSize);
> +    attribs.append(static_cast<CGLPixelFormatAttribute>(colorBits));
> +    attribs.append(kCGLPFADepthSize);
> +    attribs.append(static_cast<CGLPixelFormatAttribute>(depthBits));

So much better!

> +PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create()
> +{
> +    PassOwnPtr<GraphicsContext3D> context = new GraphicsContext3D();

I think you should use an OwnPtr on the stack here.

> +    setPixelFormat(attribs, 32, 32, true, true, false);

> +	   setPixelFormat(attribs, 32, 32, true, false, false);

> +	       setPixelFormat(attribs, 32, 16, true, false, false);

> +		   setPixelFormat(attribs, 32, 16, false, false, true);

This would be even more readable if we had enum values rather than 'true',
'false' here. But OK for now (since the called method is just above).

r=me