RESOLVED FIXED 29664
[Chromium] Add initial V8 bindings for WebGL
https://bugs.webkit.org/show_bug.cgi?id=29664
Summary [Chromium] Add initial V8 bindings for WebGL
Kenneth Russell
Reported 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.
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-
Revised patch (83.52 KB, patch)
2009-09-23 23:15 PDT, Kenneth Russell
dglazkov: review-
Revised patch (83.93 KB, patch)
2009-09-25 15:15 PDT, Kenneth Russell
no flags
Kenneth Russell
Comment 1 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.
Dimitri Glazkov (Google)
Comment 2 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.
Kenneth Russell
Comment 3 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.
Dimitri Glazkov (Google)
Comment 4 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 :)
Kenneth Russell
Comment 5 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.
Dimitri Glazkov (Google)
Comment 6 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.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2009-09-25 19:13:28 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.