Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(385)

Issue 2767923002: Always use an async TaskScheduler in TestBrowserThreadBundle. (Closed)

Created:
3 years, 9 months ago by fdoray
Modified:
3 years, 8 months ago
Reviewers:
robliao, gab, sky, mmenke
CC:
chromium-reviews, jam, darin-cc_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Always 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -128 lines) Patch
M chrome/browser/chromeos/policy/device_status_collector_browsertest.cc View 1 2 3 4 5 6 5 chunks +5 lines, -9 lines 0 comments Download
M chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc View 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_unittest.cc View 1 2 3 4 5 6 7 8 9 10 chunks +10 lines, -18 lines 0 comments Download
M chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc View 1 2 3 4 4 chunks +4 lines, -3 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 65 chunks +68 lines, -66 lines 0 comments Download
M content/public/test/test_browser_thread_bundle.h View 1 2 3 4 chunks +33 lines, -19 lines 0 comments Download
M content/public/test/test_browser_thread_bundle.cc View 1 2 3 4 3 chunks +4 lines, -9 lines 0 comments Download

Messages

Total messages: 63 (41 generated)
fdoray
PTAL
3 years, 9 months ago (2017-03-22 14:49:33 UTC) #4
gab
And the way to synchronously flush is through TaskScheduler::FlushForTesting() or RunAllBlockingPoolTasksUntilIdle() or either? Can we ...
3 years, 9 months ago (2017-03-22 16:20:47 UTC) #7
fdoray
On 2017/03/22 16:20:47, gab wrote: > And the way to synchronously flush is through TaskScheduler::FlushForTesting() ...
3 years, 8 months ago (2017-04-03 17:42:21 UTC) #9
robliao
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h#newcode20 content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in ...
3 years, 8 months ago (2017-04-03 17:53:05 UTC) #11
gab
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h#newcode20 content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks in ...
3 years, 8 months ago (2017-04-03 18:14:14 UTC) #12
robliao
lgtm https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h#newcode20 content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks ...
3 years, 8 months ago (2017-04-03 18:17:27 UTC) #13
gab
https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h#newcode30 content/public/test/test_browser_thread_bundle.h:30: // RegisterCallbackToRunOnMainThreadWhenConditionIsMet( I'm no clear on what RegisterCallbackToRunOnMainThreadWhenConditionIsMet() is ...
3 years, 8 months ago (2017-04-03 18:20:31 UTC) #14
fdoray
PTAnL https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2767923002/diff/40001/content/public/test/test_browser_thread_bundle.h#newcode20 content/public/test/test_browser_thread_bundle.h:20: // ... until there are no undelayed tasks ...
3 years, 8 months ago (2017-04-03 19:18:33 UTC) #16
gab
lgtm
3 years, 8 months ago (2017-04-03 20:01:11 UTC) #20
fdoray
sky@: PTAL
3 years, 8 months ago (2017-04-04 17:51:07 UTC) #32
fdoray
sky@: PTAL https://codereview.chromium.org/2767923002/diff/120001/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (left): https://codereview.chromium.org/2767923002/diff/120001/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc#oldcode364 chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:364: base::RunLoop().RunUntilIdle(); When a synchronous TaskScheduler was used, ...
3 years, 8 months ago (2017-04-04 17:52:55 UTC) #33
sky
LGTM
3 years, 8 months ago (2017-04-04 20:25:02 UTC) #36
fdoray
mmenke@chromium.org: Please review changes in content/browser/loader/resource_dispatcher_host_unittest.cc
3 years, 8 months ago (2017-04-04 20:34:01 UTC) #38
mmenke
https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode181 content/browser/loader/resource_dispatcher_host_unittest.cc:181: ; I think you have a few extra semi-colons ...
3 years, 8 months ago (2017-04-04 22:47:16 UTC) #39
fdoray
PTAnL https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/120001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode181 content/browser/loader/resource_dispatcher_host_unittest.cc:181: ; On 2017/04/04 22:47:16, mmenke wrote: > I ...
3 years, 8 months ago (2017-04-05 12:34:21 UTC) #42
mmenke
c/b/l LGTM https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode2618 content/browser/loader/resource_dispatcher_host_unittest.cc:2618: nit: Remove blank line.
3 years, 8 months ago (2017-04-05 15:14:16 UTC) #45
fdoray
https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader/resource_dispatcher_host_unittest.cc File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2767923002/diff/140001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode2618 content/browser/loader/resource_dispatcher_host_unittest.cc:2618: On 2017/04/05 15:14:16, mmenke wrote: > nit: Remove blank ...
3 years, 8 months ago (2017-04-05 15:45:57 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767923002/160001
3 years, 8 months ago (2017-04-05 15:46:23 UTC) #51
commit-bot: I haz the power
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_chromeos_ozone_rel_ng/builds/355550)
3 years, 8 months ago (2017-04-05 17:48:05 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767923002/160001
3 years, 8 months ago (2017-04-05 17:57:18 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767923002/180001
3 years, 8 months ago (2017-04-05 18:01:19 UTC) #60
commit-bot: I haz the power
3 years, 8 months ago (2017-04-05 19:26:45 UTC) #63
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/575c4b2eda34faaafd76af673ebe...

Powered by Google App Engine
This is Rietveld 408576698