Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523393002:
Make comments in byte_stream.cc gender neutral. (Closed)
DescriptionMake comments in byte_stream.cc gender neutral.
BUG=542537
Committed: https://crrev.com/4fdd214c8827247635fdb98ad49df8857dc6e526
Cr-Commit-Position: refs/heads/master@{#434595}
Patch Set 1 #Patch Set 2 : Fixed formatting #
Total comments: 2
Patch Set 3 : Re adds context for weak pointers #
Total comments: 3
Patch Set 4 : Removes unnecessary reformatting #
Total comments: 3
Patch Set 5 : Removes unnecessary formatting #Messages
Total messages: 30 (16 generated)
The CQ bit was checked by meredithl@google.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...
meredithl@google.com changed reviewers: + kinuko@chromium.org
Hi kinuko PTAL, thanks.
Hi kinuko PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/2523393002/diff/20001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/20001/content/browser/byte_st... content/browser/byte_stream.cc:25: // This is a RefCountedThreadSafe boolean that can be cleared in an object drive-by: I don't think we should lose this sentence. The idea that it is a "poor man's weak pointer" is important to give the reader context about what type of thing this class is trying to emulate, poorly. How about "a simplistic weak pointer" or "a poor-person's weak pointer"? (Also see email I just sent about "poor-man" in general.)
+1 to Matt's suggestion, we probably want to keep the meaning there. (Otherwise lgtm)
The CQ bit was checked by meredithl@google.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...
On 2016/11/25 09:31:38, kinuko wrote: > +1 to Matt's suggestion, we probably want to keep the meaning there. (Otherwise > lgtm) Does the term "makeshift" adequately express the necessary meaning? "poor person" rings a bit classist imo.
Updated to the term "makeshift weak pointer" https://codereview.chromium.org/2523393002/diff/20001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/20001/content/browser/byte_st... content/browser/byte_stream.cc:25: // This is a RefCountedThreadSafe boolean that can be cleared in an object On 2016/11/25 00:35:42, Matt Giuca wrote: > drive-by: I don't think we should lose this sentence. The idea that it is a > "poor man's weak pointer" is important to give the reader context about what > type of thing this class is trying to emulate, poorly. > > How about "a simplistic weak pointer" or "a poor-person's weak pointer"? (Also > see email I just sent about "poor-man" in general.) Done.
sgtm also lgtm https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... content/browser/byte_stream.cc:29: // TODO(rdsmith): A better solution would be extending weak pointers to support nit: Wrap "to support" onto the next line (no need to unnecessarily reformat lines you didn't touch).
https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... content/browser/byte_stream.cc:29: // TODO(rdsmith): A better solution would be extending weak pointers to support On 2016/11/28 00:10:41, Matt Giuca wrote: > nit: Wrap "to support" onto the next line (no need to unnecessarily reformat > lines you didn't touch). Acknowledged.
https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/40001/content/browser/byte_st... content/browser/byte_stream.cc:29: // TODO(rdsmith): A better solution would be extending weak pointers to support On 2016/11/28 00:10:41, Matt Giuca wrote: > nit: Wrap "to support" onto the next line (no need to unnecessarily reformat > lines you didn't touch). Done.
https://codereview.chromium.org/2523393002/diff/60001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/60001/content/browser/byte_st... content/browser/byte_stream.cc:33: LifetimeFlag() : is_alive(true) {} nit: There's some more unnecessary formatting changes here. Put back the space in between these braces.
https://codereview.chromium.org/2523393002/diff/60001/content/browser/byte_st... File content/browser/byte_stream.cc (right): https://codereview.chromium.org/2523393002/diff/60001/content/browser/byte_st... content/browser/byte_stream.cc:33: LifetimeFlag() : is_alive(true) {} On 2016/11/28 00:51:34, Matt Giuca wrote: > nit: There's some more unnecessary formatting changes here. Put back the space > in between these braces. My bad, I accidentally ran clang in sublime over that part as well. https://codereview.chromium.org/2523393002/diff/60001/content/browser/byte_st... content/browser/byte_stream.cc:33: LifetimeFlag() : is_alive(true) {} On 2016/11/28 00:51:34, Matt Giuca wrote: > nit: There's some more unnecessary formatting changes here. Put back the space > in between these braces. Done.
The CQ bit was checked by meredithl@google.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 meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2523393002/#ps80001 (title: "Removes unnecessary formatting")
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": 80001, "attempt_start_ts": 1480302647548380,
"parent_rev": "850743e49727d6bd66e38fe05aa7cc76c0b9066d", "commit_rev":
"ce3a3b046d10d2fa8b013ff8eb783a4369846439"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make comments in byte_stream.cc gender neutral. BUG=542537 ========== to ========== Make comments in byte_stream.cc gender neutral. BUG=542537 Committed: https://crrev.com/4fdd214c8827247635fdb98ad49df8857dc6e526 Cr-Commit-Position: refs/heads/master@{#434595} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4fdd214c8827247635fdb98ad49df8857dc6e526 Cr-Commit-Position: refs/heads/master@{#434595} |
