Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810543002:
[tests] Convert a detached typed array to a null string (Closed)
|
Created:
3 years, 8 months ago by Choongwoo Han Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[tests] Convert a detached typed array to a null string
Converting a detached typed array to a String should throw a TypeError
according to spec. Thus, this patch manually converts detached typed
arrays to null strings without calling TypedArray.prototype.toString.
BUG=v8:4648, v8:4665, v8:4953
Review-Url: https://codereview.chromium.org/2810543002
Cr-Commit-Position: refs/heads/master@{#463681}
Committed: https://chromium.googlesource.com/chromium/src/+/2fb66ab65463f429142b4d259f64728bbdae4bc3
Patch Set 1 #
Total comments: 4
Patch Set 2 : clean up #
Total comments: 2
Patch Set 3 : remove a comment #
Messages
Total messages: 29 (17 generated)
cwhan.tunz@gmail.com changed reviewers: + littledan@chromium.org
Can you take a look? or, do I have to add chromium reviewers?
lgtm LGTM with nits fixed https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html (right): https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:45: } Nit: this code could be slightly cleaned up, and clarified in the test results what's going on (you would need test expectation updates), with something like function convertDetached(obj) { // Represent detached TypedArrays as "" if (obj instanceOf Int8Array && obj.byteLength === 0) return "[detached TypedArray]"; return obj; } testPassed("Posting message ('" + convertDetached(first) + "', " + third.map(convertDetached) + ")" + reason); https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:52: function tryPostMessage(first, second, third, shouldFail) { tryPostMessageFunction(window.postMessage, first, second, third, shouldFail); Nit: Undo this whitespace change.
https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html (right): https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:45: } On 2017/04/11 10:20:22, Dan Ehrenberg wrote: > Nit: this code could be slightly cleaned up, and clarified in the test results > what's going on (you would need test expectation updates), with something like > > function convertDetached(obj) { > // Represent detached TypedArrays as "" > if (obj instanceOf Int8Array && obj.byteLength === 0) > return "[detached TypedArray]"; > return obj; > } > > testPassed("Posting message ('" + convertDetached(first) + "', " + > third.map(convertDetached) + ")" + reason); Done. https://codereview.chromium.org/2810543002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:52: function tryPostMessage(first, second, third, shouldFail) { tryPostMessageFunction(window.postMessage, first, second, third, shouldFail); On 2017/04/11 10:20:22, Dan Ehrenberg wrote: > Nit: Undo this whitespace change. Done.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by littledan@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [tests] Convert a detached typed array to a null string Converting a detached typed array to a String should throw a TypeError according to spec. Thus, this patch manually converts detached typed arrays to null strings without calling TypedArray.prototype.toString. BUG=v8:4648, v8:4665, v8:4953 ========== to ========== [tests] Convert a detached typed array to a null string Converting a detached typed array to a String should throw a TypeError according to spec. Thus, this patch manually converts detached typed arrays to null strings without calling TypedArray.prototype.toString. BUG=v8:4648, v8:4665, v8:4953 ==========
littledan@chromium.org changed reviewers: + adamk@chromium.org
This needs a review from a Chromium committer. Adding adamk as a reviewer.
https://codereview.chromium.org/2810543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html (right): https://codereview.chromium.org/2810543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:29: // Represent detached TypedArrays as "" This comment doesn't match what the code does.
https://codereview.chromium.org/2810543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html (right): https://codereview.chromium.org/2810543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/Window/window-postmessage-args.html:29: // Represent detached TypedArrays as "" On 2017/04/11 15:13:33, adamk wrote: > This comment doesn't match what the code does. Done.
lgtm
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cwhan.tunz@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2810543002/#ps40001 (title: "remove a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491932833167350,
"parent_rev": "b265af93a6c5594f08c92a93a1c06cb0611c775d", "commit_rev":
"2fb66ab65463f429142b4d259f64728bbdae4bc3"}
Message was sent while issue was closed.
Description was changed from ========== [tests] Convert a detached typed array to a null string Converting a detached typed array to a String should throw a TypeError according to spec. Thus, this patch manually converts detached typed arrays to null strings without calling TypedArray.prototype.toString. BUG=v8:4648, v8:4665, v8:4953 ========== to ========== [tests] Convert a detached typed array to a null string Converting a detached typed array to a String should throw a TypeError according to spec. Thus, this patch manually converts detached typed arrays to null strings without calling TypedArray.prototype.toString. BUG=v8:4648, v8:4665, v8:4953 Review-Url: https://codereview.chromium.org/2810543002 Cr-Commit-Position: refs/heads/master@{#463681} Committed: https://chromium.googlesource.com/chromium/src/+/2fb66ab65463f429142b4d259f64... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2fb66ab65463f429142b4d259f64... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
