Bug 29664 - [Chromium] Add initial V8 bindings for WebGL
Summary: [Chromium] Add initial V8 bindings for WebGL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-22 17:33 PDT by Kenneth Russell
Modified: 2009-12-15 14:41 PST (History)
4 users (show)

See Also:


Attachments
Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions. (83.82 KB, patch)
2009-09-22 17:40 PDT, Kenneth Russell
dglazkov: review-
Details | Formatted Diff | Diff
Revised patch (83.52 KB, patch)
2009-09-23 23:15 PDT, Kenneth Russell
dglazkov: review-
Details | Formatted Diff | Diff
Revised patch (83.93 KB, patch)
2009-09-25 15:15 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.