|
|
DescriptionSupport external task cancellation mechanisms in base::Callback::IsCancelled
This CL cleans up the implementation of base::Callback::IsCancelled(),
and exposes an injection point as BindCancellationChecker, so that
external code can implement the cancellation handling.
Plus, this CL adds a specialization of BindCancellationChecker for
blink::TaskRunner::Runner, so that the blink task scheduler can handle a
cancellation of a task posted via WebTaskRunner::postCancellableTask() properly.
Committed: https://crrev.com/6c92eabcfe60a3cfd43b51cedec7ce41061a5d62
Cr-Commit-Position: refs/heads/master@{#434511}
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : . #Patch Set 4 : s/BindCancellationChecker/CallbackCancellationTraits/ #
Total comments: 3
Patch Set 5 : . #
Messages
Total messages: 43 (31 generated)
The CQ bit was checked by tzik@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: This issue passed the CQ dry run.
The CQ bit was checked by tzik@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 ========== Support external task cancellation mechanisms in base::Callback::IsCancelled BUG= ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Also, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ==========
Description was changed from ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Also, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled 123456789012345678901234567890123456789012345678901234567890123456789012 This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ==========
Description was changed from ========== Support external task cancellation mechanisms in base::Callback::IsCancelled 123456789012345678901234567890123456789012345678901234567890123456789012 This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ==========
tzik@chromium.org changed reviewers: + alexclarke@chromium.org
tzik@chromium.org changed reviewers: + dcheng@chromium.org
Description was changed from ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker to implement it externally. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ==========
tzik@chromium.org changed reviewers: + haraken@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think dcheng or alex would be a better reviewer for this. https://codereview.chromium.org/2487493004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc (right): https://codereview.chromium.org/2487493004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc:118: std::deque<base::Closure> FakeWebTaskRunner::takePendingTasks() { takePendingTasksForTesting ?
https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h#new... base/bind_helpers.h:534: struct BindCancellationChecker { My main thoughts are around naming. Most of our injection points for Bind / Callback are named SomethingSomethingTraits. For consistency in naming, it might be nice to keep a similar theme here. Maybe CallbackCancellationTraits? Since it's a Callback (which is the result of a Bind) that is cancellable. https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h#new... base/bind_helpers.h:548: static bool Run(const Functor&, const Receiver& receiver, const Args&...) { Similarly, I think calling this Run() is a bit confusing (since it overlaps with Callback::Run). Would it be OK to just call this IsCancelled()?
The CQ bit was checked by tzik@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 tzik@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 ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. BUG= ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. ==========
https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h#new... base/bind_helpers.h:534: struct BindCancellationChecker { On 2016/11/10 06:29:23, dcheng wrote: > My main thoughts are around naming. > > Most of our injection points for Bind / Callback are named > SomethingSomethingTraits. For consistency in naming, it might be nice to keep a > similar theme here. Maybe CallbackCancellationTraits? Since it's a Callback > (which is the result of a Bind) that is cancellable. Done. https://codereview.chromium.org/2487493004/diff/20001/base/bind_helpers.h#new... base/bind_helpers.h:548: static bool Run(const Functor&, const Receiver& receiver, const Args&...) { On 2016/11/10 06:29:23, dcheng wrote: > Similarly, I think calling this Run() is a bit confusing (since it overlaps with > Callback::Run). Would it be OK to just call this IsCancelled()? Done. https://codereview.chromium.org/2487493004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc (right): https://codereview.chromium.org/2487493004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc:118: std::deque<base::Closure> FakeWebTaskRunner::takePendingTasks() { On 2016/11/10 04:20:10, haraken wrote: > > takePendingTasksForTesting ? Done. ... but is it useful to add "ForTesting" to a Fake* class in a test/ directory?
LGTM https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h#new... base/bind_helpers.h:528: // base::Bind. Nit: I'd make this comment mostly descriptive about the purpose (allow customization of callback's cancellation semantics) and fix the references to Run() below. // CallbackCancellationTraits allows customization of Callback's // cancellation semantics. By default, callbacks are not cancellable. // A specialization should set is_cancellable = true and implement an // IsCancelled() that returns if the callback should be cancelled.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h#new... base/bind_helpers.h:528: // base::Bind. On 2016/11/10 07:36:23, dcheng wrote: > Nit: I'd make this comment mostly descriptive about the purpose (allow > customization of callback's cancellation semantics) and fix the references to > Run() below. > > // CallbackCancellationTraits allows customization of Callback's > // cancellation semantics. By default, callbacks are not cancellable. > // A specialization should set is_cancellable = true and implement an > // IsCancelled() that returns if the callback should be cancelled. +1
https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2487493004/diff/60001/base/bind_helpers.h#new... base/bind_helpers.h:528: // base::Bind. On 2016/11/10 07:36:23, dcheng wrote: > Nit: I'd make this comment mostly descriptive about the purpose (allow > customization of callback's cancellation semantics) and fix the references to > Run() below. > > // CallbackCancellationTraits allows customization of Callback's > // cancellation semantics. By default, callbacks are not cancellable. > // A specialization should set is_cancellable = true and implement an > // IsCancelled() that returns if the callback should be cancelled. Done.
The CQ bit was checked by tzik@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: This issue passed the CQ dry run.
haraken: ping. Any comment or concern?
On 2016/11/25 09:48:31, tzik wrote: > haraken: ping. Any comment or concern? LGTM
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2487493004/#ps80001 (title: ".")
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": 1480075926591270, "parent_rev": "521a6026895909072c09e3d7fa21ddeaf7d9c30c", "commit_rev": "2d9db450918bcc7e56b63e9696b05f1c36fa6cb7"}
Message was sent while issue was closed.
Description was changed from ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. ========== to ========== Support external task cancellation mechanisms in base::Callback::IsCancelled This CL cleans up the implementation of base::Callback::IsCancelled(), and exposes an injection point as BindCancellationChecker, so that external code can implement the cancellation handling. Plus, this CL adds a specialization of BindCancellationChecker for blink::TaskRunner::Runner, so that the blink task scheduler can handle a cancellation of a task posted via WebTaskRunner::postCancellableTask() properly. Committed: https://crrev.com/6c92eabcfe60a3cfd43b51cedec7ce41061a5d62 Cr-Commit-Position: refs/heads/master@{#434511} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6c92eabcfe60a3cfd43b51cedec7ce41061a5d62 Cr-Commit-Position: refs/heads/master@{#434511} |