[webkit-reviews] review denied: [Bug 53940] Implement the OES_vertex_array_object WebGL extension : [Attachment 81569] Implementation of OES_vertex_array_object for WebKit/OSX (reupload)

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Ben Vanik <ben.vanik at gmail.com>'s
request for review:
Bug 53940: Implement the OES_vertex_array_object WebGL extension
https://bugs.webkit.org/show_bug.cgi?id=53940

Attachment 81569: Implementation of OES_vertex_array_object for WebKit/OSX
(reupload)
https://bugs.webkit.org/attachment.cgi?id=81569&action=review

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

Great work. The patch looks almost perfect. r- for a few relatively minor
issues. It's generally difficult to land a patch via the commit queue that
modifies project.pbxproj because there's a lot of churn in that file. Ping me
when you're ready to upload a new patch and we can try to get it landed
quickly.

> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

There are slight differences among the copyright notices in the new files.
Could you please normalize them all to what's in Source/WebKit/LICENSE?

> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:65
> +    return o;

Prefer "return o.release();".

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1381
> +	   if (loc >= 0 && loc < (int)m_maxVertexAttribs) {

Use static_cast<int>(...).

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:41
> +    static PassRefPtr<WebGLVertexArrayObjectOES>
create(WebGLRenderingContext*, bool);

Recently there's been a lot of discussion on webkit-dev about preferring
descriptive enums instead of bools. Please define an enum (perhaps something
like "NormalVAO", "DefaultVAO"?) and pass it instead of the bool. You could
give it a default value so most of the call sites don't have to supply it.

> Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.h:44
> +    class VertexAttribState {

Make this a struct instead of a class, since everything's public. Everything
else (in particular the constructor) can remain.

> Source/WebCore/platform/graphics/Extensions3D.h:116
> +    virtual Platform3DObject createVertexArrayOES() = 0;
> +    virtual void deleteVertexArrayOES(Platform3DObject) = 0;
> +    virtual GC3Dboolean isVertexArrayOES(Platform3DObject) = 0;
> +    virtual void bindVertexArrayOES(Platform3DObject) = 0;

You'll need to stub these out in
Source/WebCore/platform/graphics/qt/Extensions3DQt.{cpp,h}. I'm not sure why
the Qt EWS bot compiled this patch successfully -- maybe they're not currently
compiling WebGL.

> Source/WebCore/platform/graphics/GraphicsTypes3D.h:26
> +#ifndef GraphicsTypes3D_h

This header needs to be added to a couple more of the build systems, in
particular Source/WebCore/WebCore.pro and Source/WebCore/WebCore.gypi. Even
though compilation didn't fail, the dependencies might not be correct.