[webkit-reviews] review denied: [Bug 45557] Define GC3D types to match GL types and use them in GraphicsContext3D : [Attachment 78030] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 45557: Define GC3D types to match GL types and use them in
GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=45557

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

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

This cleanup mostly looks good and I definitely think it's the right thing to
do. While it's huge it will make maintaining this code easier in the future. As
we discussed, the style errors are because arguments have names in the headers,
but because in OpenGL there's so much information encoded in those names, I
think they should be kept despite the style violations. There are a few changes
that need to be made.

> WebCore/html/canvas/WebGLRenderingContext.cpp:2333
> -    unsigned long componentsPerPixel, bytesPerComponent;
> +    unsigned int componentsPerPixel, bytesPerComponent;

See below about using one of the new typedefs here.

> WebCore/html/canvas/WebGLRenderingContext.cpp:3859
> -    unsigned long componentsPerPixel, bytesPerComponent;
> +    unsigned int componentsPerPixel, bytesPerComponent;

Here as well.

> WebCore/platform/graphics/GraphicsContext3D.cpp:46
> +    unsigned int bytesPerComponent(GC3Denum type)

Use one of the new typedefs for the return type; specifically, GC3Duint.

> WebCore/platform/graphics/GraphicsContext3D.cpp:62
> +    unsigned int componentsPerPixel(GC3Denum format, GC3Denum type)

Return type GC3Duint.

> WebCore/platform/graphics/GraphicsContext3D.cpp:90
> +    unsigned int imageSizeInBytes(GC3Dsizei width, GC3Dsizei height,
GC3Denum format, GC3Denum type)

Use GC3Duint as the return type.

> WebCore/platform/graphics/GraphicsContext3D.cpp:142
> +							  uint32_t*
componentsPerPixel,
> +							  uint32_t*
bytesPerComponent)

Use one of your new typedefs instead of uint32_t; specifically, GC3Duint.

> WebCore/platform/graphics/GraphicsContext3D.h:43
> +typedef void GC3Dvoid;

I don't think the GC3Dvoid typedef adds any value. The gl2.h header doesn't
reference its own GLvoid typedef. I would recommend just removing it and
continuing to use const void* for blocks of data throughout.

> WebCore/platform/graphics/GraphicsContext3D.h:52
> +typedef signed long int GC3Dsizeiptr;

For completeness add GC3Dintptr.

> WebCore/platform/graphics/GraphicsContext3D.h:606
> +    void bufferData(GC3Denum target, GC3Dsizei size, GC3Denum usage);
> +    void bufferData(GC3Denum target, GC3Dsizei size, const GC3Dvoid* data,
GC3Denum usage);

To match the gl2.h header, the GC3Dsizei uses here should be GC3Dsizeiptr.

> WebCore/platform/graphics/GraphicsContext3D.h:607
> +    void bufferSubData(GC3Denum target, GC3Dsizeiptr offset, GC3Dsizei size,
const GC3Dvoid* data);

To match gl2.h, offset should be a GC3Dintptr, and size should be a
GC3Dsizeiptr.

> WebCore/platform/graphics/GraphicsContext3D.h:630
> +    void drawElements(GC3Denum mode, GC3Dsizei count, GC3Denum type,
GC3Dsizeiptr offset);

I think offset should be a GC3Dintptr.

> WebCore/platform/graphics/GraphicsContext3D.h:700
> +    bool texImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat,
GC3Dsizei width, GC3Dsizei height, GC3Dint border, GC3Denum format, GC3Denum
type, GC3Dvoid* pixels);

I note that gl2.h uses GLint for internalformat but I think your choice of
GC3Denum is better.

Should we take this opportunity to add "const" to the pixels argument?

> WebCore/platform/graphics/GraphicsContext3D.h:703
> +    void texSubImage2D(GC3Denum target, GC3Dint level, GC3Dint xoffset,
GC3Dint yoffset, GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum
type, GC3Dvoid* pixels);

Same question about making pixels const void*.

> WebCore/platform/graphics/GraphicsContext3D.h:723
> +    void uniform1f(GC3Dint location, GC3Dfloat x);
> +    void uniform1fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size);
> +    void uniform1i(GC3Dint location, GC3Dint x);
> +    void uniform1iv(GC3Dint location, GC3Dint* v, GC3Dsizei size);
> +    void uniform2f(GC3Dint location, GC3Dfloat x, float y);
> +    void uniform2fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size);
> +    void uniform2i(GC3Dint location, GC3Dint x, GC3Dint y);
> +    void uniform2iv(GC3Dint location, GC3Dint* v, GC3Dsizei size);
> +    void uniform3f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z);

> +    void uniform3fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size);
> +    void uniform3i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z);
> +    void uniform3iv(GC3Dint location, GC3Dint* v, GC3Dsizei size);
> +    void uniform4f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z,
GC3Dfloat w);
> +    void uniform4fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size);
> +    void uniform4i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z,
GC3Dint w);
> +    void uniform4iv(GC3Dint location, GC3Dint* v, GC3Dsizei size);
> +    void uniformMatrix2fv(GC3Dint location, GC3Dboolean transpose,
GC3Dfloat* value, GC3Dsizei size);
> +    void uniformMatrix3fv(GC3Dint location, GC3Dboolean transpose,
GC3Dfloat* value, GC3Dsizei size);
> +    void uniformMatrix4fv(GC3Dint location, GC3Dboolean transpose,
GC3Dfloat* value, GC3Dsizei size);

Could you add a FIXME about changing the argument order to these methods to
match OpenGL's? The discrepancy is historical and there's no good reason for
it.

> WebCore/platform/graphics/GraphicsContext3D.h:737
> +				GC3Dsizei stride, GC3Dsizeiptr offset);

As above I think offset should be GC3Dintptr.