Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2836963002:
Allow ScopedTaskEnvironment to pump UI or IO messages on the main thread. (Closed)
DescriptionAllow ScopedTaskEnvironment to pump UI or IO messages on the main thread.
This change will allow tests that require a main thread that
pumps UI or IO messages to be migrated to ScopedTaskEnvironment.
BUG=708584
Review-Url: https://codereview.chromium.org/2836963002
Cr-Commit-Position: refs/heads/master@{#467436}
Committed: https://chromium.googlesource.com/chromium/src/+/8744ec289206bffe0393619be6bef8322301b169
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 1
Patch Set 3 : no_ui_by_default #
Total comments: 1
Messages
Total messages: 25 (16 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...
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...
Description was changed from ========== Switch from MessageLoop to MessageLoopForUI in ScopedTaskEnvironment. This change will allow tests that currently instantiate a MessageLoopForUI on the main thread to be migrated to ScopedTaskEnvironment. This change makes sense since the Chrome browser process has a MessageLoopForUI on its main thread. BUG=708584 ========== to ========== Parametrize main MessageLoop type in ScopedTaskEnvironment. This change will allow tests that require a MessageLoopForUI or a MessageLoopForIO to be migrated to ScopedTaskEnvironment. To match the Chrome browser process and TestBrowserThreadBundle, ScopedTaskEnvironment will initialize a MessageLoopForUI by default. BUG=708584 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2836963002/diff/20001/base/test/scoped_task_e... File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2836963002/diff/20001/base/test/scoped_task_e... base/test/scoped_task_environment.h:42: ScopedTaskEnvironment(MainThreadType main_thread_type = MainThreadType::UI); The default message loop was used before (does not pump UI messages). Should the default be one that also pumps UI messages?
I'm leaning towards a not lgtm for this as it will prevent a migration from an underlying MessageLoop to a TestMockTimeTaskRunner once RunLoop supports both. I'd prefer if users of an explicit MessageLoopForUI|IO kept using that explicitly (until we have a generic solution for that too for TaskScheduler).
Had misread intent, thought (without reading code) this was suggesting to pass MessageLoopForUI/IO directly. Parametrizing as this CL is actually doing lgtm.
Description was changed from ========== Parametrize main MessageLoop type in ScopedTaskEnvironment. This change will allow tests that require a MessageLoopForUI or a MessageLoopForIO to be migrated to ScopedTaskEnvironment. To match the Chrome browser process and TestBrowserThreadBundle, ScopedTaskEnvironment will initialize a MessageLoopForUI by default. BUG=708584 ========== to ========== Allow ScopedTaskEnvironment to pump UI or IO messages on the main thread. This change will allow tests that require a main thread that pumps UI or IO messages to be migrated to ScopedTaskEnvironment. BUG=708584 ==========
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2836963002/#ps40001 (title: "no_ui_by_default")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493233210146270,
"parent_rev": "23c6ab6fce7255b8795b18c7d77fc6abefa86dc9", "commit_rev":
"8744ec289206bffe0393619be6bef8322301b169"}
Message was sent while issue was closed.
Description was changed from ========== Allow ScopedTaskEnvironment to pump UI or IO messages on the main thread. This change will allow tests that require a main thread that pumps UI or IO messages to be migrated to ScopedTaskEnvironment. BUG=708584 ========== to ========== Allow ScopedTaskEnvironment to pump UI or IO messages on the main thread. This change will allow tests that require a main thread that pumps UI or IO messages to be migrated to ScopedTaskEnvironment. BUG=708584 Review-Url: https://codereview.chromium.org/2836963002 Cr-Commit-Position: refs/heads/master@{#467436} Committed: https://chromium.googlesource.com/chromium/src/+/8744ec289206bffe0393619be6be... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8744ec289206bffe0393619be6be...
Message was sent while issue was closed.
https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_e... File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:36: // The main thread doesn't pump messages. s/doesn't pump messages/doesn't pump system messages/ ?
Message was sent while issue was closed.
On 2017/04/27 14:22:59, gab wrote: > https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_e... > File base/test/scoped_task_environment.h (right): > > https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_e... > base/test/scoped_task_environment.h:36: // The main thread doesn't pump > messages. > s/doesn't pump messages/doesn't pump system messages/ ? Done https://codereview.chromium.org/2847773002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
