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

Issue 2880453003: Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. (Closed)

Created:
3 years, 7 months ago by gab
Modified:
3 years, 7 months ago
CC:
chromium-reviews, sadrul, vmpstr+watch_chromium.org, wfh+watch_chromium.org, blink-reviews, tracing+reviews_chromium.org, danakj+watch_chromium.org, scheduler-bugs_chromium.org, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. Separates RunLoop/MessageLoop further. Still some interdependencies to address but this drops friend class RunLoop; from MessageLoop :). It also moves the relevant state from TLS back onto the MessageLoop instance (hidden as private data in RunLoop::Delegate). This is easier to manage and ensures cleanup when MessageLoop goes away (particularly important in unit tests). BUG=703346, 719530 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=phajdan.jr@chromium.org for content\public\test\cache_test_util.h fwd-decl nit Review-Url: https://codereview.chromium.org/2880453003 Cr-Commit-Position: refs/heads/master@{#472695} Committed: https://chromium.googlesource.com/chromium/src/+/27355196d32f75606b3e43b54bd0d03ef42b4579

Patch Set 1 #

Total comments: 6

Patch Set 2 : update dependency #

Patch Set 3 : update dependency again #

Patch Set 4 : Split Delegate public interface to Delegate::Client. #

Patch Set 5 : self-review: comment nit #

Patch Set 6 : rebase on r472282 and fix new lacking message_loop.h #

Patch Set 7 : +missing fwd-decl in cache_test_util.h #

Patch Set 8 : Allow IsRunning/IsNestedOnCurrentThread() without a registered Delegate #

Patch Set 9 : oops, re-invert IsRunning logic #

Unified diffs Side-by-side diffs Delta from patch set Stats (+212 lines, -144 lines) Patch
M base/message_loop/message_loop.h View 1 2 3 7 chunks +9 lines, -10 lines 0 comments Download
M base/message_loop/message_loop.cc View 1 2 3 10 chunks +27 lines, -24 lines 0 comments Download
M base/run_loop.h View 1 2 3 4 5 6 7 5 chunks +100 lines, -17 lines 0 comments Download
M base/run_loop.cc View 1 2 3 4 5 6 7 8 7 chunks +70 lines, -93 lines 0 comments Download
M components/download/internal/scheduler/network_listener_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/test/cache_test_util.h View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 68 (46 generated)
gab
Dana PTaL :), thanks!
3 years, 7 months ago (2017-05-12 21:06:43 UTC) #7
danakj
https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/message_loop.h File base/message_loop/message_loop.h (right): https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/message_loop.h#newcode351 base/message_loop/message_loop.h:351: // RunLoop::Delegate: nit: "RunLoop::Delegate implementation." ? https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h File base/run_loop.h ...
3 years, 7 months ago (2017-05-15 16:23:13 UTC) #10
gab
Replies below, thanks https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/message_loop.h File base/message_loop/message_loop.h (right): https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/message_loop.h#newcode351 base/message_loop/message_loop.h:351: // RunLoop::Delegate: On 2017/05/15 16:23:13, danakj ...
3 years, 7 months ago (2017-05-15 17:28:35 UTC) #11
danakj
On Mon, May 15, 2017 at 1:28 PM, <gab@chromium.org> wrote: > Replies below, thanks > ...
3 years, 7 months ago (2017-05-15 17:37:12 UTC) #12
danakj
On Mon, May 15, 2017 at 1:28 PM, <gab@chromium.org> wrote: > Replies below, thanks > ...
3 years, 7 months ago (2017-05-15 17:37:13 UTC) #13
gab
On 2017/05/15 17:37:13, danakj wrote: > On Mon, May 15, 2017 at 1:28 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> ...
3 years, 7 months ago (2017-05-15 20:08:31 UTC) #14
danakj
On 2017/05/15 20:08:31, gab wrote: > On 2017/05/15 17:37:13, danakj wrote: > > On Mon, ...
3 years, 7 months ago (2017-05-15 21:15:31 UTC) #28
gab
PTanL, managed to split the interfaces without introducing heap objects and while still using a ...
3 years, 7 months ago (2017-05-16 18:00:26 UTC) #31
danakj
LGTM :)
3 years, 7 months ago (2017-05-16 18:34:25 UTC) #36
gab
TBR xingliu for network_listener_unittest.cc include fix
3 years, 7 months ago (2017-05-17 01:57:57 UTC) #41
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/2880453003/140001
3 years, 7 months ago (2017-05-17 01:58:28 UTC) #42
xingliu
lgtm
3 years, 7 months ago (2017-05-17 02:00:23 UTC) #43
commit-bot: I haz the power
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_linux/builds/369653) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 7 months ago (2017-05-17 02:18:04 UTC) #45
gab
TBR=phajdan.jr@chromium.org for content\public\test\cache_test_util.h fwd-decl nit
3 years, 7 months ago (2017-05-17 02:53:43 UTC) #48
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/2880453003/160001
3 years, 7 months ago (2017-05-17 02:57:17 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/428737)
3 years, 7 months ago (2017-05-17 06:50:12 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/2880453003/180001
3 years, 7 months ago (2017-05-17 14:56:17 UTC) #56
commit-bot: I haz the power
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_linux/builds/370032)
3 years, 7 months ago (2017-05-17 16:32:14 UTC) #58
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/2880453003/200001
3 years, 7 months ago (2017-05-17 16:55:32 UTC) #61
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/429535)
3 years, 7 months ago (2017-05-17 21:58:32 UTC) #63
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/2880453003/200001
3 years, 7 months ago (2017-05-18 03:57:50 UTC) #65
commit-bot: I haz the power
3 years, 7 months ago (2017-05-18 06:01:28 UTC) #68
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/27355196d32f75606b3e43b54bd0...

Powered by Google App Engine
This is Rietveld 408576698