Bug 35612 – Update WebGLArray.slice() to new spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The implementation of WebGLArray.slice() needs to be updated for the revised spec, in particular to treat its arguments as (start, end) instead of (offset, length).

[Zhenyao Mo]

Created attachment 50987 [details]
patch

[WebKit Review Bot]

Attachment 50987 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/html/canvas/WebGLIntArray.cpp:75:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLUnsignedShortArray.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLShortArray.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLFloatArray.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLUnsignedIntArray.cpp:75:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLByteArray.cpp:73:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/html/canvas/WebGLUnsignedByteArray.cpp:75:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 7 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

Created attachment 50991 [details]
revised patch: fix style issues

[Oliver Hunt]

Comment on attachment 50991 [details]
revised patch: fix style issues

> +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

[Zhenyao Mo]

Created attachment 51168 [details]
revised patch : responding to Oliver Hunt's review

The boundary check for offset/length happens in slice() implementation of each WebGL*Array class, so there is no need to do it in calculateOffsetAndLength().

[Kenneth Russell]

Latest patch looks good to me.

[Oliver Hunt]

Comment on attachment 51168 [details]
revised patch : responding to Oliver Hunt's review

r=me, but i've noticed that
unsigned fullOffset = m_byteOffset + offset * sizeof(/*whatever type*/);
can overflow and therefore lead to incorrect _behaviour_ as clampOffsetAndNumElements is unaware of the initial byteoffset so you can trigger an overflow that will produce fullOffset < m_byteOffset

[WebKit Commit Bot]

Comment on attachment 51168 [details]
revised patch : responding to Oliver Hunt's review

Clearing flags on attachment: 51168

Committed r56291: <http://trac.webkit.org/changeset/56291>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zhenyao Mo]

Created attachment 51654 [details]
revised patch: add explanation in the changeLog.

[Zhenyao Mo]

ignore the last patch.  it belongs to another bug.  sorry about the confusion.