Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2818963002:
Comment out asserts in WritableStream.js (Closed)
DescriptionComment out asserts in WritableStream.js
assert() statements can have a major performance impact, so comment them out.
They have not been removed completely as they can be easily re-enabled for
debugging purposes, and are also useful as documentation.
BUG=709832
Review-Url: https://codereview.chromium.org/2818963002
Cr-Commit-Position: refs/heads/master@{#465538}
Committed: https://chromium.googlesource.com/chromium/src/+/8ac2349b4fc1649f4fef0a69de517ad74acabc10
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Indent fix #Messages
Total messages: 19 (9 generated)
ricea@chromium.org changed reviewers: + domenic@chromium.org
This depends on https://codereview.chromium.org/2823563002 which I want to do a bit more work on before sending for review. But since this one is easy you can go ahead and review it first.
lgtm with the caveat that I can't figure out how to view the file after-patch in the web UI, and I haven't taken the time to download the patchset locally. What I wanted to do was Ctrl+F through the final file for TEMP_ASSERT to check that you caught everything. Assuming you've done that yourself, all changes in the diff look good.
On 2017/04/14 17:45:18, domenic wrote: > lgtm with the caveat that I can't figure out how to view the file after-patch in > the web UI, and I haven't taken the time to download the patchset locally. What > I wanted to do was Ctrl+F through the final file for TEMP_ASSERT to check that > you caught everything. Assuming you've done that yourself, all changes in the > diff look good. One thing you can do is click "View" on the file you want and then "Expand All" on one of the blue "...skipping" lines. It doesn't do exactly what you want, because it shows both versions at the same time, but you can Ctrl-F through it.
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from domenic@chromium.org Link to the patchset: https://codereview.chromium.org/2818963002/#ps20001 (title: "Rebase")
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.
ricea@chromium.org changed reviewers: + tyoshino@chromium.org
+tyoshino for OWNERS.
lgtm https://codereview.chromium.org/2818963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/streams/WritableStream.js (right): https://codereview.chromium.org/2818963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/streams/WritableStream.js:263: // '! WritableStreamHasOperationMarkedInFlight(_stream_) is *false*'); indent
The CQ bit was checked by ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, domenic@chromium.org Link to the patchset: https://codereview.chromium.org/2818963002/#ps40001 (title: "Indent fix")
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": 1492586884560470,
"parent_rev": "fcbfde6ea277a07cddc3db613a3b4ff3de5d3933", "commit_rev":
"8ac2349b4fc1649f4fef0a69de517ad74acabc10"}
Message was sent while issue was closed.
Description was changed from ========== Comment out asserts in WritableStream.js assert() statements can have a major performance impact, so comment them out. They have not been removed completely as they can be easily re-enabled for debugging purposes, and are also useful as documentation. BUG=709832 ========== to ========== Comment out asserts in WritableStream.js assert() statements can have a major performance impact, so comment them out. They have not been removed completely as they can be easily re-enabled for debugging purposes, and are also useful as documentation. BUG=709832 Review-Url: https://codereview.chromium.org/2818963002 Cr-Commit-Position: refs/heads/master@{#465538} Committed: https://chromium.googlesource.com/chromium/src/+/8ac2349b4fc1649f4fef0a69de51... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8ac2349b4fc1649f4fef0a69de51...
Message was sent while issue was closed.
https://codereview.chromium.org/2818963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/streams/WritableStream.js (right): https://codereview.chromium.org/2818963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/streams/WritableStream.js:263: // '! WritableStreamHasOperationMarkedInFlight(_stream_) is *false*'); On 2017/04/19 07:13:32, tyoshino wrote: > indent Done. |
