[webkit-reviews] review denied: [Bug 35612] Update WebGLArray.slice() to new spec : [Attachment 50991] revised patch: fix style issues

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Zhenyao Mo <zmo at google.com>'s request
for review:
Bug 35612: Update WebGLArray.slice() to new spec
https://bugs.webkit.org/show_bug.cgi?id=35612

Attachment 50991: revised patch: fix style issues
https://bugs.webkit.org/attachment.cgi?id=50991&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
> +JSValue JSWebGLArray::slice(ExecState* exec, const ArgList& args)
> +{
> +    WebGLArray* array = reinterpret_cast<WebGLArray*>(impl());
> +
> +    long start, end;
> +    switch (args.size()) {
> +    case 0:
...
> +    case 2:
remove case 2 -- the default case will do fine.  The same issue exists in the
V8 binding.
> +    default:
> +	   start = args.at(0).toInt32(exec);
> +	   end = args.at(1).toInt32(exec);
> +    }
> +    return toJS(exec, globalObject(), array->slice(start, end));
> +}
> +


> Index: WebCore/html/canvas/WebGLArray.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLArray.cpp	(revision 56135)
> +++ WebCore/html/canvas/WebGLArray.cpp	(working copy)
> @@ -58,6 +58,23 @@ void WebGLArray::setImpl(WebGLArray* arr
>      memcpy(base + byteOffset, array->baseAddress(), array->byteLength());
>  }
>  
> +void WebGLArray::calculateOffsetAndLength(long start, long end, unsigned
arraySize,
> +					     unsigned* offset, unsigned*
length)
> +{
> +    if (start < 0)
> +	   start += arraySize;
> +    if (start < 0)
> +	   start = 0;
> +    if (end < 0)
> +	   end += arraySize;
> +    if (end < 0)
> +	   end = 0;
> +    if (end < start)
> +	   end = start;
> +    *offset = static_cast<unsigned>(start);
> +    *length = static_cast<unsigned>(end - start);
What is the size of long vs size of unsigned?  I don't like this mismatch of
types with potentially different sizes and signedness.

Also there doesn't appear to be any attempt to ensure that end of the slice is
not off the end of the array