|
|
DescriptionAlways use an async TaskScheduler in TestBrowserThreadBundle.
Motivation:
- The upcoming ScopedTaskEnvironment will always use an async
TaskScheduler.
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit?usp=sharing
- As part of the blocking pool to TaskScheduler migration, we would
like to turn on redirection of blocking pool tasks to TaskScheduler
in TestBrowserThreadBundle. Many tests fail if we do that and
TaskScheduler runs tasks synchronously (asynchronous execution is
closer to what we had before).
BUG=667892
Review-Url: https://codereview.chromium.org/2767923002
Cr-Commit-Position: refs/heads/master@{#462175}
Committed: https://chromium.googlesource.com/chromium/src/+/575c4b2eda34faaafd76af673ebee4a97e933569
Patch Set 1 #Patch Set 2 : CR #Patch Set 3 : self-review #
Total comments: 10
Patch Set 4 : CR-robliao-gab-13-14 #Patch Set 5 : fix-test-errors #Patch Set 6 : fix-test-errors #Patch Set 7 : fix-test-errors #
Total comments: 3
Patch Set 8 : CR-mmenke-remove-extra-semi-colons #
Total comments: 2
Patch Set 9 : CR-mmenke-remove-blank-line #Patch Set 10 : fix-test-error #
Messages
Total messages: 63 (41 generated)
The CQ bit was checked by fdoray@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...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
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...)
And the way to synchronously flush is through TaskScheduler::FlushForTesting() or RunAllBlockingPoolTasksUntilIdle() or either? Can we document that better in test_browser_thread_bundle.h?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
On 2017/03/22 16:20:47, gab wrote: > And the way to synchronously flush is through TaskScheduler::FlushForTesting() > or RunAllBlockingPoolTasksUntilIdle() or either? > > Can we document that better in test_browser_thread_bundle.h? Done. PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in the shared MessageLoop: Should there be this many ways to flush tasks or will this be simplified later (do tests today take advantage of these distinctions)?
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in the shared MessageLoop: On 2017/04/03 17:53:05, robliao wrote: > Should there be this many ways to flush tasks or will this be simplified later > (do tests today take advantage of these distinctions)? > I think this distinction matters (and I'm making it in ScopedTaskEnvironment). Ultimately there should "run main thread" and "run all" I think. Anything in between (i.e. run a specific sequence) ought to be done by manually posting WaitableEvents IMO.
lgtm https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in the shared MessageLoop: On 2017/04/03 18:14:14, gab wrote: > On 2017/04/03 17:53:05, robliao wrote: > > Should there be this many ways to flush tasks or will this be simplified later > > (do tests today take advantage of these distinctions)? > > > > I think this distinction matters (and I'm making it in ScopedTaskEnvironment). > Ultimately there should "run main thread" and "run all" I think. Anything in > between (i.e. run a specific sequence) ought to be done by manually posting > WaitableEvents IMO. Presented that way, I might be able to buy it. We can have this discussion on a separate thread.
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // RegisterCallbackToRunOnMainThreadWhenConditionIsMet( I'm no clear on what RegisterCallbackToRunOnMainThreadWhenConditionIsMet() is pointing at in this example and I'm sure it'll be confusing to avg dev. https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:31: // base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop))); QuitClosure https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:36: // base::TaskScheduler()->FlushForTesting(); // Note: base::SequencedWorkerPool::FlushForTesting() is equivalent but deprecated. (to enlighten readers of old code wondering why what's used there isn't listed here)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in the shared MessageLoop: On 2017/04/03 18:17:27, robliao wrote: > On 2017/04/03 18:14:14, gab wrote: > > On 2017/04/03 17:53:05, robliao wrote: > > > Should there be this many ways to flush tasks or will this be simplified > later > > > (do tests today take advantage of these distinctions)? > > > > > > > I think this distinction matters (and I'm making it in ScopedTaskEnvironment). > > Ultimately there should "run main thread" and "run all" I think. Anything in > > between (i.e. run a specific sequence) ought to be done by manually posting > > WaitableEvents IMO. > > Presented that way, I might be able to buy it. We can have this discussion on a > separate thread. I expect most that most of the comments added in this CL will become obsolete when ScopedTaskEnvironment is introduced. https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // RegisterCallbackToRunOnMainThreadWhenConditionIsMet( On 2017/04/03 18:20:30, gab wrote: > I'm no clear on what RegisterCallbackToRunOnMainThreadWhenConditionIsMet() is > pointing at in this example and I'm sure it'll be confusing to avg dev. Done. https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:31: // base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop))); On 2017/04/03 18:20:30, gab wrote: > QuitClosure Done. https://codereview.chromium.org/2767923002/diff/40001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:36: // base::TaskScheduler()->FlushForTesting(); On 2017/04/03 18:20:30, gab wrote: > // Note: base::SequencedWorkerPool::FlushForTesting() is equivalent but > deprecated. > > (to enlighten readers of old code wondering why what's used there isn't listed > here) Done.
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm
The CQ bit was checked by fdoray@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_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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...
fdoray@chromium.org changed reviewers: + sky@chromium.org
sky@: PTAL
sky@: PTAL https://codereview.chromium.org/2767923002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (left): https://codereview.chromium.org/2767923002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:364: base::RunLoop().RunUntilIdle(); When a synchronous TaskScheduler was used, base::RunLoop().RunUntilIdle() indirectly flushed TaskScheduler. Now, content::RunAllBlockingPoolTasksUntilIdle() is required to flush blocking pool + task scheduler + main MessageLoop.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
fdoray@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@chromium.org: Please review changes in content/browser/loader/resource_dispatcher_host_unittest.cc
https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:181: ; I think you have a few extra semi-colons in this file.
The CQ bit was checked by fdoray@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...
PTAnL https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:181: ; On 2017/04/04 22:47:16, mmenke wrote: > I think you have a few extra semi-colons in this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
c/b/l LGTM https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:2618: nit: Remove blank line.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2767923002/#ps160001 (title: "CR-mmenke-remove-blank-line")
https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:2618: On 2017/04/05 15:14:16, mmenke wrote: > nit: Remove blank line. Done.
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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 checked by fdoray@chromium.org to run a CQ dry run
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org, sky@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2767923002/#ps180001 (title: "fix-test-error")
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": 180001, "attempt_start_ts": 1491415252762070, "parent_rev": "7a3b81227dcba0c840b6c5bf1c92b252c2503e90", "commit_rev": "575c4b2eda34faaafd76af673ebee4a97e933569"}
Message was sent while issue was closed.
Description was changed from ========== Always use an async TaskScheduler in TestBrowserThreadBundle. Motivation: - The upcoming ScopedTaskEnvironment will always use an async TaskScheduler. https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... - As part of the blocking pool to TaskScheduler migration, we would like to turn on redirection of blocking pool tasks to TaskScheduler in TestBrowserThreadBundle. Many tests fail if we do that and TaskScheduler runs tasks synchronously (asynchronous execution is closer to what we had before). BUG=667892 ========== to ========== Always use an async TaskScheduler in TestBrowserThreadBundle. Motivation: - The upcoming ScopedTaskEnvironment will always use an async TaskScheduler. https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... - As part of the blocking pool to TaskScheduler migration, we would like to turn on redirection of blocking pool tasks to TaskScheduler in TestBrowserThreadBundle. Many tests fail if we do that and TaskScheduler runs tasks synchronously (asynchronous execution is closer to what we had before). BUG=667892 Review-Url: https://codereview.chromium.org/2767923002 Cr-Commit-Position: refs/heads/master@{#462175} Committed: https://chromium.googlesource.com/chromium/src/+/575c4b2eda34faaafd76af673ebe... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/575c4b2eda34faaafd76af673ebe... |