Bug 29664 – [Chromium] Add initial V8 bindings for WebGL

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Need to add the initial V8 bindings for the WebGL implementation already in WebKit. Refer to the following bugs for more information:

https://bugs.webkit.org/show_bug.cgi?id=28018
https://bugs.webkit.org/show_bug.cgi?id=29010

This patch also slightly changes how WebCore/platform/graphics/GraphicsContext3D.h abstracts out its platform-dependent portions.

[Kenneth Russell]

Created attachment 39968 [details]
Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions.

[Dimitri Glazkov (Google)]

Comment on attachment 39968 [details]
Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions.

Welcome to the club, kbr! :)

Here's your first round of hazing. I only looked at style so far. Let's get that out of the way first.

> +    if (argLen != 1) {
> +        return throwError("Wrong number of arguments specified to constructor (requires 1)");
> +    }

No braces for one-liner conditions.

> +    int len = 0;
> +    if (!args[0]->IsInt32()) {
> +        return throwError("Argument to CanvasArrayBuffer constructor was not an integer");
> +    }

Ditto.

> +        if (argLen == 0) {
> +            return throwError("No arguments specified to constructor");
> +        }

Ditto.

> +
> +        // See whether the first argument is a CanvasArrayBuffer.
> +        if (V8CanvasArrayBuffer::HasInstance(args[0])) {
> +            if (argLen > 3) {
> +                return throwError("Wrong number of arguments to new Canvas<T>Array(CanvasArrayBuffer, int, int)");
> +            }

Ditto.

> +            CanvasArrayBuffer* buf =
> +                V8DOMWrapper::convertToNativeObject<CanvasArrayBuffer>(V8ClassIndex::CANVASARRAYBUFFER,
> +                                                                       args[0]->ToObject());

BTW, no need to constrain to 80 cols. WebKit style allows you to run free.

> +            if (buf == NULL) {
> +                return throwError("Could not convert argument 0 to a CanvasArrayBuffer");
> +            }

No braces.

> +            int offset = 0;
> +            if (argLen > 1) {
> +                offset = toInt32(args[1], ok);
> +                if (!ok) {
> +                    return throwError("Could not convert argument 1 to an integer");
> +                }

Ditto.

> +            if (argLen > 2) {
> +                length = toInt32(args[2], ok);
> +                if (!ok) {
> +                    return throwError("Could not convert argument 2 to an integer");
> +                }

Ditto.

> +            }
> +            if (length < 0) {
> +                return throwError("Length / offset out of range");

Ditto and elsewhere.

> +                if (!val->IsNumber()) {
> +                    char buf[256];
> +                    sprintf(buf, "Could not convert array element %d to a number", i);

Probably shouldn't be sprintf'ing. LOG(...), perhaps?

> +    if ((index < 0) || (index >= byteBuffer->length()))

too many parens here and elsewhere.

> +    if ((index >= 0) && (index < array->length())) {

Ditto.

> +    float* data;
> +    if (!tryFastMalloc(len * sizeof(float)).getValue(data)) {
> +        return NULL;

return 0; here and elsewhere.

[Kenneth Russell]

Created attachment 40047 [details]
Revised patch

Throughout the patch, fixed the style issues that were pointed out.

sprintf is being used to construct an informative exception message for JavaScript. Logging would not produce the desired results.

[Dimitri Glazkov (Google)]

Comment on attachment 40047 [details]
Revised patch

We're almost there!

> +        if (argLen == 0) {
> +            return throwError("No arguments specified to constructor");
> +        }

no braces for one-liners.

> +        } else {
> +            return throwError("Could not convert argument 0 to either an int32 or an array");
> +        }

ditto.

> +    if (data == NULL) {

if (!data) {

> +    if (data == NULL) {

ditto.

> +    if (data == NULL) {

ditto.

> +        OwnPtr<GraphicsContext3DInternal> d;

would love a slightly more descriptive name :)

[Kenneth Russell]

Created attachment 40150 [details]
Revised patch

Made above requested changes. Comments:

> > +        } else {
> > +            return throwError("Could not convert argument 0 to either an int32 or an array");
> > +        }
> 
> ditto.

Changed this, but I think leaving the braces off the trailing else clause is ugly.

> > +        OwnPtr<GraphicsContext3DInternal> d;
> 
> would love a slightly more descriptive name :)

Renamed to m_internal.

[Dimitri Glazkov (Google)]

Comment on attachment 40150 [details]
Revised patch

r=me.

Marking cq+ as this shouldn't break anything, thanks to the compile flag.

[WebKit Commit Bot]

Comment on attachment 40150 [details]
Revised patch

Clearing flags on attachment: 40150

Committed r48781: <http://trac.webkit.org/changeset/48781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.