Bug 35619 - Implement memmove behavior for WebGLArray set()
Summary: Implement memmove behavior for WebGLArray set()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 16:14 PST by Kenneth Russell
Modified: 2010-03-31 03:26 PDT (History)
5 users (show)

See Also:


Attachments
patch (6.77 KB, patch)
2010-03-24 14:45 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: tiny fix (6.56 KB, patch)
2010-03-24 14:48 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: add explanation in the changeLog. (6.68 KB, patch)
2010-03-25 10:21 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : fixed the typo Ken Russell pointed out (6.68 KB, patch)
2010-03-30 18:22 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-03-02 16:14:00 PST
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.
Comment 1 Zhenyao Mo 2010-03-24 14:45:31 PDT
Created attachment 51544 [details]
patch
Comment 2 Zhenyao Mo 2010-03-24 14:48:39 PDT
Created attachment 51546 [details]
revised patch: tiny fix
Comment 3 Eric Seidel (no email) 2010-03-25 01:13:46 PDT
Comment on attachment 51546 [details]
revised patch: tiny fix

Your ChangeLog should really explain *why* this is a good idea.
Comment 4 Zhenyao Mo 2010-03-25 10:21:34 PDT
Created attachment 51655 [details]
revised patch: add explanation in the changeLog.
Comment 5 Kenneth Russell 2010-03-30 16:58:28 PDT
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]?
Comment 6 Zhenyao Mo 2010-03-30 18:22:04 PDT
Created attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out
Comment 7 Kenneth Russell 2010-03-30 18:23:57 PDT
Comment on attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

Looks good.
Comment 8 Darin Fisher (:fishd, Google) 2010-03-30 22:40:08 PDT
Comment on attachment 52113 [details]
revised patch : fixed the typo Ken Russell pointed out

r=me
Comment 9 WebKit Commit Bot 2010-03-31 03:26:44 PDT
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>
Comment 10 WebKit Commit Bot 2010-03-31 03:26:50 PDT
All reviewed patches have been landed.  Closing bug.