Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(147)

Issue 2836963002: Allow ScopedTaskEnvironment to pump UI or IO messages on the main thread. (Closed)

Created:
1 year, 10 months ago by fdoray
Modified:
1 year, 10 months ago
Reviewers:
robliao, gab
CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/8744ec289206bffe0393619be6bef8322301b169

Patch Set 1 #

Patch Set 2 : self-review #

Total comments: 1

Patch Set 3 : no_ui_by_default #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -2 lines) Patch
M base/test/scoped_task_environment.h View 1 2 1 chunk +11 lines, -1 line 1 comment Download
M base/test/scoped_task_environment.cc View 1 2 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 25 (16 generated)
fdoray
PTAL
1 year, 10 months ago (2017-04-24 17:06:13 UTC) #11
robliao
https://codereview.chromium.org/2836963002/diff/20001/base/test/scoped_task_environment.h File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2836963002/diff/20001/base/test/scoped_task_environment.h#newcode42 base/test/scoped_task_environment.h:42: ScopedTaskEnvironment(MainThreadType main_thread_type = MainThreadType::UI); The default message loop was ...
1 year, 10 months ago (2017-04-24 22:42:31 UTC) #12
gab
I'm leaning towards a not lgtm for this as it will prevent a migration from ...
1 year, 10 months ago (2017-04-25 15:31:24 UTC) #13
gab
Had misread intent, thought (without reading code) this was suggesting to pass MessageLoopForUI/IO directly. Parametrizing ...
1 year, 10 months ago (2017-04-26 18:49:36 UTC) #14
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/2836963002/40001
1 year, 10 months ago (2017-04-26 19:00:47 UTC) #19
robliao
lgtm
1 year, 10 months ago (2017-04-26 19:18:26 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8744ec289206bffe0393619be6bef8322301b169
1 year, 10 months ago (2017-04-26 20:23:42 UTC) #23
gab
https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_environment.h File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2836963002/diff/40001/base/test/scoped_task_environment.h#newcode36 base/test/scoped_task_environment.h:36: // The main thread doesn't pump messages. s/doesn't pump ...
1 year, 10 months ago (2017-04-27 14:22:59 UTC) #24
fdoray
1 year, 10 months ago (2017-04-27 14:44:41 UTC) #25
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

Powered by Google App Engine
This is Rietveld 408576698