Bug 29664

Summary: [Chromium] Add initial V8 bindings for WebGL
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dglazkov, kbr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions.
dglazkov: review-
Revised patch
dglazkov: review-
Revised patch none

Description Kenneth Russell 2009-09-22 17:33:54 PDT
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.
Comment 1 Kenneth Russell 2009-09-22 17:40:10 PDT
Created attachment 39968 [details]
Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions.
Comment 2 Dimitri Glazkov (Google) 2009-09-23 13:24:57 PDT
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.
Comment 3 Kenneth Russell 2009-09-23 23:15:23 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2009-09-25 10:10:18 PDT
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 :)
Comment 5 Kenneth Russell 2009-09-25 15:15:13 PDT
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.
Comment 6 Dimitri Glazkov (Google) 2009-09-25 16:04:47 PDT
Comment on attachment 40150 [details]
Revised patch

r=me.

Marking cq+ as this shouldn't break anything, thanks to the compile flag.
Comment 7 WebKit Commit Bot 2009-09-25 19:13:24 PDT
Comment on attachment 40150 [details]
Revised patch

Clearing flags on attachment: 40150

Committed r48781: <http://trac.webkit.org/changeset/48781>
Comment 8 WebKit Commit Bot 2009-09-25 19:13:28 PDT
All reviewed patches have been landed.  Closing bug.