Bug 35619 – Implement memmove behavior for WebGLArray set()

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

[Kenneth Russell]

In the method WebGL<T>Array.set(WebGL<T>Array, offset) it is possible for the source and destination arrays to overlap the same region in the WebGLArrayBuffer. Per the revised spec, we need to implement memmove semantics for this case and write tests for it.

[Zhenyao Mo]

Created attachment 51544 [details]
patch

[Zhenyao Mo]

Created attachment 51546 [details]
revised patch: tiny fix

[Eric Seidel (no email)]

Comment on attachment 51546 [details]
revised patch: tiny fix

Your ChangeLog should really explain *why* this is a good idea.

[Zhenyao Mo]

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

[Kenneth Russell]

Comment on attachment 51655 [details]
revised patch: add explanation in the changeLog.

Looks good overall. One comment on the test case.

> Index: LayoutTests/fast/canvas/webgl/array-setters.html
> ===================================================================
> --- LayoutTests/fast/canvas/webgl/array-setters.html	(revision 56446)
> +++ LayoutTests/fast/canvas/webgl/array-setters.html	(working copy)
> @@ -39,6 +39,29 @@ function testSetters(typeName, low, high
>      webGLArray.set(1, array[1]);
>      shouldBe("webGLArray[1]", "array[1]");
>      shouldBe("webGLArray.get(1)", "array[1]");
> +
> +    // Verify set() behaves correctly with shared underlying buffer. 
> +    array = [0, 1, 2, 3, 4, 5];
> +    webGLArray = new type(6);
> +    webGLArray.set(array);
> +    array = webGLArray.slice(2, 4);
> +    array[0] = 88;
> +    array[1] = 99;
> +    shouldBe("webGLArray[2]", "88");
> +    shouldBe("webGLArray[3]", "99");
> +    // pre-overlap
> +    webGLArray.set(array, 1);
> +    shouldBe("webGLArray[1]", "88");
> +    shouldBe("webGLArray[2]", "99");
> +    shouldBe("array[0]", "99");
> +    shouldBe("array[1]", "99");
> +    array[1] = 77;
> +    // post-overlap
> +    webGLArray.set(array, 3);
> +    shouldBe("webGLArray[3]", "99");
> +    shouldBe("webGLArray[4]", "77");
> +    shouldBe("array[0]", "99");
> +    shouldBe("array[0]", "99");

The line above looks like a typo. Should it be verifying array[1]?

[Zhenyao Mo]

Created attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

[Kenneth Russell]

Comment on attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

Looks good.

[Darin Fisher (:fishd, Google)]

Comment on attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

r=me

[WebKit Commit Bot]

Comment on attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

Clearing flags on attachment: 52113

Committed r56833: <http://trac.webkit.org/changeset/56833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.