Bug 35619

Summary:

Implement memmove behavior for WebGLArray set()

Product:

WebKit

Reporter:

Kenneth Russell <kbr>

Component:

WebGL

Assignee:

Zhenyao Mo <zmo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarrin, commit-queue, dglazkov, fishd, oliver

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	none
revised patch: tiny fix	none
revised patch: add explanation in the changeLog.	none
revised patch : fixed the typo Ken Russell pointed out	none

Kenneth Russell

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Zhenyao Mo

Comment 1 2010-03-24 14:45:31 PDT

Created attachment 51544 [details] patch

Zhenyao Mo

Comment 2 2010-03-24 14:48:39 PDT

Created attachment 51546 [details] revised patch: tiny fix

Eric Seidel (no email)

Comment 3 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.

Zhenyao Mo

Comment 4 2010-03-25 10:21:34 PDT

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

Kenneth Russell

Comment 5 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]?

Zhenyao Mo

Comment 6 2010-03-30 18:22:04 PDT

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

Kenneth Russell

Comment 7 2010-03-30 18:23:57 PDT

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

Darin Fisher (:fishd, Google)

Comment 8 2010-03-30 22:40:08 PDT

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

WebKit Commit Bot

Comment 9 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>

WebKit Commit Bot

Comment 10 2010-03-31 03:26:50 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.