[webkit-reviews] review denied: [Bug 29664] [Chromium] Add initial V8 bindings for WebGL : [Attachment 39968] Adds V8 bindings for WebGL implementation and modifies how GraphicsContext3D abstracts platform-dependent portions.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Kenneth Russell
<kbr at google.com>'s request for review:
Bug 29664: [Chromium] Add initial V8 bindings for WebGL
https://bugs.webkit.org/show_bug.cgi?id=29664

Attachment 39968: Adds V8 bindings for WebGL implementation and modifies how
GraphicsContext3D abstracts platform-dependent portions.
https://bugs.webkit.org/attachment.cgi?id=39968&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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::CANVASARRA
YBUFFER,
> +									 
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.