|
|
DescriptionDisable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown.
Until it's possible for async observation services to post non-BLOCK_SHUTDOWN
tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky.
clang-format off is required or I couldn't find a way to make it happy with the
comment (either inside or outside the #if)
BUG=728235, 698140
Review-Url: https://codereview.chromium.org/2916673003
Cr-Commit-Position: refs/heads/master@{#476799}
Committed: https://chromium.googlesource.com/chromium/src/+/b2b72df4bdc0217e7ed7cc321648028a429edf61
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix BlockShutdown/TaskSchedulerTaskTrackerTest.WillPostAfterShutdown #
Total comments: 3
Messages
Total messages: 28 (18 generated)
Description was changed from ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. BUG=728235, 698140 ========== to ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. BUG=728235, 698140 ==========
gab@chromium.org changed reviewers: + fdoray@chromium.org, robliao@chromium.org
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Description was changed from ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. BUG=728235, 698140 ========== to ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. clang-format off is required or I couldn't find a way to make it happy with the comment (either inside or outside the #if) BUG=728235, 698140 ==========
As discussed.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm + nit https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:413: // FileDescriptorWatcher. Same is true for FilePathWatcher Tab this one more to the right
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:413: // FileDescriptorWatcher. Same is true for FilePathWatcher On 2017/05/31 21:33:54, robliao wrote: > Tab this one more to the right Why? This is aligned with the code inside the if as it should be.
The CQ bit was checked by gab@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.
https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2916673003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:413: // FileDescriptorWatcher. Same is true for FilePathWatcher On 2017/06/01 13:52:30, gab wrote: > On 2017/05/31 21:33:54, robliao wrote: > > Tab this one more to the right > > Why? This is aligned with the code inside the if as it should be. Ah interesting! This was a rendering bug in mobile Safari where I was viewing the code. https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (left): https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:415: if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) { Instead of deleting this code, should we just disable the test?
https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (left): https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:415: if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) { On 2017/06/01 17:00:53, robliao wrote: > Instead of deleting this code, should we just disable the test? I thought that too but then realized we still need to verify it returns false which is what this now does. When we re-enable the DCHECK we will have no choice but to bring this back as this test will then crash.
lgtm https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker_unittest.cc (left): https://codereview.chromium.org/2916673003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker_unittest.cc:415: if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) { On 2017/06/01 17:59:39, gab wrote: > On 2017/06/01 17:00:53, robliao wrote: > > Instead of deleting this code, should we just disable the test? > > I thought that too but then realized we still need to verify it returns false > which is what this now does. > > When we re-enable the DCHECK we will have no choice but to bring this back as > this test will then crash. Acknowledged.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2916673003/#ps40001 (title: "fix BlockShutdown/TaskSchedulerTaskTrackerTest.WillPostAfterShutdown")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@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": 40001, "attempt_start_ts": 1496426674797170, "parent_rev": "af9e620d3dff0161d3dc9f8ef3607dfff69ca315", "commit_rev": "b2b72df4bdc0217e7ed7cc321648028a429edf61"}
Message was sent while issue was closed.
Description was changed from ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. clang-format off is required or I couldn't find a way to make it happy with the comment (either inside or outside the #if) BUG=728235, 698140 ========== to ========== Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. clang-format off is required or I couldn't find a way to make it happy with the comment (either inside or outside the #if) BUG=728235, 698140 Review-Url: https://codereview.chromium.org/2916673003 Cr-Commit-Position: refs/heads/master@{#476799} Committed: https://chromium.googlesource.com/chromium/src/+/b2b72df4bdc0217e7ed7cc321648... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b2b72df4bdc0217e7ed7cc321648... |