Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709883003:
Elaborate on the CancelableCallback docs (Closed)
DescriptionElaborate on the CancelableCallback docs
Clarify and make more precise the documentation on the
allowed and disallowed uses of the class.
BUG=694268
Patch Set 1 #
Total comments: 1
Patch Set 2 : Comment change #
Total comments: 1
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by emaxx@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...
Description was changed from ========== Elaborate CancelableCallback docs BUG=694268 ========== to ========== Elaborate on the CancelableCallback docs Clarify and make more precise the documentation on the allowed and disallowed uses of the class. BUG=694268 ==========
emaxx@chromium.org changed reviewers: + jhawkins@chromium.org
jhawkins@: could you PTAL? I'm not really sure how correct are the statements that I'm adding into the CancelableCallback documentation. The existing documentation, on one side, is too short-spoken, without really answering some important questions (like is it allowed to call Cancel() from inside the callback). On the other side, the existing documentation seems to provide a bit too strict thread safety requirements - probably, stricter than it's actually necessary. The next step, after this CL is reviewed, would be to add the missing DCHECKS and to start fixing the consumer code (I know for sure that some of the CancelableCallback uses are incorrect and were just lucky to not explode so far - for instance, in the enterprise policy related code). +mastiz@ FYI
LGTM https://codereview.chromium.org/2709883003/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): https://codereview.chromium.org/2709883003/diff/1/base/cancelable_callback.h#... base/cancelable_callback.h:16: // NOTE: nit: Maybe just change line 10 to NOTES: and remove this NOTE:.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_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) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by emaxx@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 checked by emaxx@chromium.org to run a CQ dry run
emaxx@chromium.org changed reviewers: + thakis@chromium.org
thakis@, could you PTAL?
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: Try jobs failed on following builders: cast_shell_linux 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_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_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) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/22 02:16:42, emaxx wrote: > thakis@, could you PTAL? Friendly ping. Once the documentation for CancelableCallback is clarified, I planned to start introducing DCHECK's and fixing the consumer code.
https://codereview.chromium.org/2709883003/diff/20001/base/cancelable_callback.h File base/cancelable_callback.h (right): https://codereview.chromium.org/2709883003/diff/20001/base/cancelable_callbac... base/cancelable_callback.h:24: // CancelableCallback objects are allowed to be constructed on a different Hmm, thinking more on this, this added comment is actually misleading: while it's allowed to construct CancelableCallback on a different thread, it's not allowed to _destruct_ them there (unless one can guarantee that there are no callbacks currently posted anywhere, but that's probably not a typical use case). As it's becoming quite hard to explain this here, I'm starting to think that this whole different-thread-corner-case should be simply disallowed, as before. Any thoughts on this?
On 2017/03/01 03:37:54, emaxx wrote: > https://codereview.chromium.org/2709883003/diff/20001/base/cancelable_callback.h > File base/cancelable_callback.h (right): > > https://codereview.chromium.org/2709883003/diff/20001/base/cancelable_callbac... > base/cancelable_callback.h:24: // CancelableCallback objects are allowed to be > constructed on a different > Hmm, thinking more on this, this added comment is actually misleading: while > it's allowed to construct CancelableCallback on a different thread, it's not > allowed to _destruct_ them there (unless one can guarantee that there are no > callbacks currently posted anywhere, but that's probably not a typical use > case). > > As it's becoming quite hard to explain this here, I'm starting to think that > this whole different-thread-corner-case should be simply disallowed, as before. > Any thoughts on this? Yeah, I'm a fan off keeping it strict in order to enforce simplicity :-) It's been so long, but maybe I actually knew of those issues when writing it...hard to say.
This CL will be probably completely superseded by crrev.com/2722163004. |
