Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434783002:
Use OnceClosure in TestPendingTask (Closed)
DescriptionUse OnceClosure in TestPendingTask
This CL replaces base::Callback in TestPendingTask with base::OnceClosure,
as a preparation of a similar migration of base::TaskRunner.
Review-Url: https://codereview.chromium.org/2434783002
Cr-Commit-Position: refs/heads/master@{#446583}
Committed: https://chromium.googlesource.com/chromium/src/+/a6f0007ae2a8d785d347ef9887bdd18a23605978
Patch Set 1 #Patch Set 2 : fix #
Total comments: 13
Patch Set 3 : . #Patch Set 4 : +comment #Patch Set 5 : rebase #Patch Set 6 : +comment. s/UnsafeConvertOnceClosureToRepeating/RunOnceClosure + Bind/ #Patch Set 7 : win buildfix #
Total comments: 4
Patch Set 8 : +comment #Messages
Total messages: 66 (47 generated)
Description was changed from ========== Use OnceClosure in TestPendingTask BUG= ========== to ========== Use OnceClosure in TestPendingTask BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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.
Description was changed from ========== Use OnceClosure in TestPendingTask BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Use OnceClosure in TestPendingTask BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
tzik@chromium.org changed reviewers: + danakj@chromium.org
Description was changed from ========== Use OnceClosure in TestPendingTask BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
PTAL
https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:93: DISALLOW_COPY_AND_ASSIGN(TestOrderedPendingTask); private https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:253: std::move(task_info.task).Run(); I think we should very soon write an email explainer to chromium-dev and blink-dev about OnceCallback and this syntax of using std::move() for a function call - how it works and why you have to do it, what you can expect from it. Could you take a first look at writing that? https://codereview.chromium.org/2434783002/diff/20001/base/test/test_pending_... File base/test/test_pending_task.h (right): https://codereview.chromium.org/2434783002/diff/20001/base/test/test_pending_... base/test/test_pending_task.h:67: DISALLOW_COPY_AND_ASSIGN(TestPendingTask); private https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:233: pending_tasks_.erase(task_to_run); Is it for sure valid to erase an iterator when you've const-casted it and changed its internals in a set? It seems like undefined kinda territory. https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:35: TestOrderablePendingTask(TestOrderablePendingTask&&); add operator= too
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:93: DISALLOW_COPY_AND_ASSIGN(TestOrderedPendingTask); On 2016/10/20 20:07:46, danakj wrote: > private Done. https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:253: std::move(task_info.task).Run(); On 2016/10/20 20:07:46, danakj wrote: > I think we should very soon write an email explainer to chromium-dev and > blink-dev about OnceCallback and this syntax of using std::move() for a function > call - how it works and why you have to do it, what you can expect from it. > Could you take a first look at writing that? OK, I'm writing go/callback-explainer. Will send this to chromium-dev/blink-dev. https://codereview.chromium.org/2434783002/diff/20001/base/test/test_pending_... File base/test/test_pending_task.h (right): https://codereview.chromium.org/2434783002/diff/20001/base/test/test_pending_... base/test/test_pending_task.h:67: DISALLOW_COPY_AND_ASSIGN(TestPendingTask); On 2016/10/20 20:07:46, danakj wrote: > private Done. https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:233: pending_tasks_.erase(task_to_run); On 2016/10/20 20:07:46, danakj wrote: > Is it for sure valid to erase an iterator when you've const-casted it and > changed its internals in a set? It seems like undefined kinda territory. Hmm. I think it's practically safe, since the task is not used for the ordering. Though, yes, it's cleaner to split the Closure part out of the struct... https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:35: TestOrderablePendingTask(TestOrderablePendingTask&&); On 2016/10/20 20:07:46, danakj wrote: > add operator= too Done.
LGTM https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:253: std::move(task_info.task).Run(); On 2016/10/27 08:20:49, tzik wrote: > On 2016/10/20 20:07:46, danakj wrote: > > I think we should very soon write an email explainer to chromium-dev and > > blink-dev about OnceCallback and this syntax of using std::move() for a > function > > call - how it works and why you have to do it, what you can expect from it. > > Could you take a first look at writing that? > > OK, I'm writing go/callback-explainer. Will send this to chromium-dev/blink-dev. Super great :D https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:233: pending_tasks_.erase(task_to_run); On 2016/10/27 08:20:49, tzik wrote: > On 2016/10/20 20:07:46, danakj wrote: > > Is it for sure valid to erase an iterator when you've const-casted it and > > changed its internals in a set? It seems like undefined kinda territory. > > Hmm. I think it's practically safe, since the task is not used for the ordering. > Though, yes, it's cleaner to split the Closure part out of the struct... ok can you leave a comment saying why this is okay so people notice it's sketchy if they try to change it in the future?
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: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
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: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Description was changed from ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
Let me revive this CL. dcheng: This is the other approach to migrate TestPendingTask to OnceClosure mentioned in https://codereview.chromium.org/2627863002/. Should we go this way? https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/20001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:233: pending_tasks_.erase(task_to_run); On 2016/10/27 21:15:21, danakj wrote: > On 2016/10/27 08:20:49, tzik wrote: > > On 2016/10/20 20:07:46, danakj wrote: > > > Is it for sure valid to erase an iterator when you've const-casted it and > > > changed its internals in a set? It seems like undefined kinda territory. > > > > Hmm. I think it's practically safe, since the task is not used for the > ordering. > > Though, yes, it's cleaner to split the Closure part out of the struct... > > ok can you leave a comment saying why this is okay so people notice it's sketchy > if they try to change it in the future? 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: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
I think I prefer this -- it's a bit simpler, and hopefully we can clean it up once we fix our container story. LGTM https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:192: std::move(const_cast<TestOrderedPendingTask&>(tasks_.top()))); Nit: comment here too. https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:284: *next_task = std::move(const_cast<TestOrderedPendingTask&>(tasks_.top())); And here.
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: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Adding ortuno@ and mmenke@ for OWNERS review. PTAL to: ortuno: //device/bluetooth mmenke: //net https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:192: std::move(const_cast<TestOrderedPendingTask&>(tasks_.top()))); On 2017/01/24 09:15:52, dcheng wrote: > Nit: comment here too. Done. https://codereview.chromium.org/2434783002/diff/120001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:284: *next_task = std::move(const_cast<TestOrderedPendingTask&>(tasks_.top())); On 2017/01/24 09:15:52, dcheng wrote: > And here. Done.
tzik@chromium.org changed reviewers: + mmenke@chromium.org, ortuno@chromium.org
...! Forgot to add them to "Reviews" form. Adding ortuno@ and mmenke@ for OWNERS review. PTAL to: ortuno: //device/bluetooth mmenke: //net
On 2017/01/24 14:20:27, tzik wrote: > ...! Forgot to add them to "Reviews" form. > > Adding ortuno@ and mmenke@ for OWNERS review. PTAL to: > ortuno: //device/bluetooth > mmenke: //net net/ LGTM
tzik@chromium.org changed reviewers: + scheib@chromium.org
ortuno seems not around. scheib: Could you ptal to //device/bluetooth change?
lgtm
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2434783002/#ps140001 (title: "+comment")
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
Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
The CQ bit was checked by tzik@chromium.org
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
Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
The CQ bit was checked by tzik@chromium.org
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
Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Description was changed from ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. ==========
The CQ bit was checked by tzik@chromium.org
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": 140001, "attempt_start_ts": 1485489299831060,
"parent_rev": "451a5bb2b7ad113ceaae90237a0c314336a32ecb", "commit_rev":
"a6f0007ae2a8d785d347ef9887bdd18a23605978"}
Message was sent while issue was closed.
Description was changed from ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. ========== to ========== Use OnceClosure in TestPendingTask This CL replaces base::Callback in TestPendingTask with base::OnceClosure, as a preparation of a similar migration of base::TaskRunner. Review-Url: https://codereview.chromium.org/2434783002 Cr-Commit-Position: refs/heads/master@{#446583} Committed: https://chromium.googlesource.com/chromium/src/+/a6f0007ae2a8d785d347ef9887bd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a6f0007ae2a8d785d347ef9887bd... |
