|
|
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. |
DescriptionIntroduce 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 #
Dependent Patchsets: Messages
Total messages: 68 (46 generated)
Description was changed from ========== Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. BUG= ========== to ========== Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. BUG=703346, 719530 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gab@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 ========== Introduce RunLoop::Delegate splitting RunLoop/MessageLoop some more. BUG=703346, 719530 ========== to ========== 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 ==========
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTaL :), thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/messa... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/messa... 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 (right): https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h#newcode123 base/run_loop.h:123: class BASE_EXPORT Delegate { While its less # of classes to do this, this Delegate is combining two things into one which I feel is awkward: 1. Operations that RunLoop wants to delegate to another class (Run/Quit). 2. Private APIs for the delegate to use from RunLoop. IOW half the methods are for RunLoop to use and half are for the Delegate to use. What if we split them and did something like class Delegate { Init(Private*) Run() = 0; Quit() = 0; } class Private { GetTopMostRunLoop() { ... } IsNested() { ... } } etc.. This also I think gets us more like having interface (abstract APIs) separate from implementations which has a better code smell to me. wdyt? https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h#newcode146 base/run_loop.h:146: friend class RunLoop; This isn't needed since the class is nested right?
Replies below, thanks https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/messa... File base/message_loop/message_loop.h (right): https://codereview.chromium.org/2880453003/diff/20001/base/message_loop/messa... base/message_loop/message_loop.h:351: // RunLoop::Delegate: On 2017/05/15 16:23:13, danakj wrote: > nit: "RunLoop::Delegate implementation." ? I've been told to do the opposite (this CL's version) many times and the paradigm is all over the codebase so I've now adopted it per less chars. https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h File base/run_loop.h (right): https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h#newcode123 base/run_loop.h:123: class BASE_EXPORT Delegate { On 2017/05/15 16:23:13, danakj wrote: > While its less # of classes to do this, this Delegate is combining two things > into one which I feel is awkward: > 1. Operations that RunLoop wants to delegate to another class (Run/Quit). > 2. Private APIs for the delegate to use from RunLoop. > > IOW half the methods are for RunLoop to use and half are for the Delegate to > use. > > What if we split them and did something like > > class Delegate { > Init(Private*) > Run() = 0; > Quit() = 0; > } > > class Private { > GetTopMostRunLoop() { ... } > IsNested() { ... } > } > > etc.. > > This also I think gets us more like having interface (abstract APIs) separate > from implementations which has a better code smell to me. > > wdyt? Hmmm, I don't see how this would work, what is MessageLoop implementing? Are you okay with the fact that the Delegate holds state that is really meant to be used by RunLoop? I agree it's a bit of a weird indirection but it's the best ownership model I could think of. The state is really all on the Delegate (only one per thread). RunLoop instances are merely dedicated drivers of that state. As for the protected Delegate methods that allow MessageLoop to poke back into that private RunLoop state which it owns but doesn't have access to... IsNested() could be implemented MessageLoop side, it's just that we already the information so poking at it directly seemed simpler. GetTopMostRunLoop() will go away as I split this even further. https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h#newcode146 base/run_loop.h:146: friend class RunLoop; On 2017/05/15 16:23:13, danakj wrote: > This isn't needed since the class is nested right? In Java you're right I think but in C++ it's required.
On Mon, May 15, 2017 at 1:28 PM, <gab@chromium.org> wrote: > 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 wrote: > > nit: "RunLoop::Delegate implementation." ? > > I've been told to do the opposite (this CL's version) many times and the > paradigm is all over the codebase so I've now adopted it per less chars. > > https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h > File base/run_loop.h (right): > > https://codereview.chromium.org/2880453003/diff/20001/ > base/run_loop.h#newcode123 > base/run_loop.h:123: class BASE_EXPORT Delegate { > On 2017/05/15 16:23:13, danakj wrote: > > While its less # of classes to do this, this Delegate is combining two > things > > into one which I feel is awkward: > > 1. Operations that RunLoop wants to delegate to another class > (Run/Quit). > > 2. Private APIs for the delegate to use from RunLoop. > > > > IOW half the methods are for RunLoop to use and half are for the > Delegate to > > use. > > > > What if we split them and did something like > > > > class Delegate { > > Init(Private*) > > Run() = 0; > > Quit() = 0; > > } > > > > class Private { > > GetTopMostRunLoop() { ... } > > IsNested() { ... } > > } > > > > etc.. > > > > This also I think gets us more like having interface (abstract APIs) > separate > > from implementations which has a better code smell to me. > > > > wdyt? > > Hmmm, I don't see how this would work, what is MessageLoop implementing? > I guess we need a created instance per messageloop so itd be like class MessageLoop : public RunLoop::Delegate { // Delegate Init(unique_ptr<RunLoop::Private>); Run(); Quit(); unique_ptr<RunLoop::Private> run_loop_stuff_; } This way MessageLoop holds parts of RunLoop essentially, but doesn't subclass parts of RunLoop. wdyt? > Are you okay with the fact that the Delegate holds state that is really > meant to be used by RunLoop? > I'm okay with something holding state that is used by RunLoop. But I think that Delegate interface should just be an interface that RunLoop wants to call to do things. The other stuff used by the Delegate to implement itself could be separate as per above. That feels like a cleaner separation to me. > I agree it's a bit of a weird indirection > but it's the best ownership model I could think of. The state is really > all on the Delegate (only one per thread). > If it works and feels nicer, RunLoop could instead inherit the "Private" stuff above and give a raw pointer to itself to MessageLoop instead of mallocing a Private thing with access to RunLoop internals. > RunLoop instances are merely dedicated drivers of that state. > > As for the protected Delegate methods that allow MessageLoop to poke > back into that private RunLoop state which it owns but doesn't have > access to... IsNested() could be implemented MessageLoop side, it's just > that we already the information so poking at it directly seemed simpler. > GetTopMostRunLoop() will go away as I split this even further. > > https://codereview.chromium.org/2880453003/diff/20001/ > base/run_loop.h#newcode146 > base/run_loop.h:146: friend class RunLoop; > On 2017/05/15 16:23:13, danakj wrote: > > This isn't needed since the class is nested right? > > In Java you're right I think but in C++ it's required. > This compiles for instance: class Outer { public: Outer(int i) : i_(i) {} class Inner { public: Inner(Outer* o) { std::cout << o->i_ << '\n'; } }; private: int i_; }; int main() { Outer o(7); Outer::Inner i(&o); return 0; } -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Mon, May 15, 2017 at 1:28 PM, <gab@chromium.org> wrote: > 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 wrote: > > nit: "RunLoop::Delegate implementation." ? > > I've been told to do the opposite (this CL's version) many times and the > paradigm is all over the codebase so I've now adopted it per less chars. > > https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h > File base/run_loop.h (right): > > https://codereview.chromium.org/2880453003/diff/20001/ > base/run_loop.h#newcode123 > base/run_loop.h:123: class BASE_EXPORT Delegate { > On 2017/05/15 16:23:13, danakj wrote: > > While its less # of classes to do this, this Delegate is combining two > things > > into one which I feel is awkward: > > 1. Operations that RunLoop wants to delegate to another class > (Run/Quit). > > 2. Private APIs for the delegate to use from RunLoop. > > > > IOW half the methods are for RunLoop to use and half are for the > Delegate to > > use. > > > > What if we split them and did something like > > > > class Delegate { > > Init(Private*) > > Run() = 0; > > Quit() = 0; > > } > > > > class Private { > > GetTopMostRunLoop() { ... } > > IsNested() { ... } > > } > > > > etc.. > > > > This also I think gets us more like having interface (abstract APIs) > separate > > from implementations which has a better code smell to me. > > > > wdyt? > > Hmmm, I don't see how this would work, what is MessageLoop implementing? > I guess we need a created instance per messageloop so itd be like class MessageLoop : public RunLoop::Delegate { // Delegate Init(unique_ptr<RunLoop::Private>); Run(); Quit(); unique_ptr<RunLoop::Private> run_loop_stuff_; } This way MessageLoop holds parts of RunLoop essentially, but doesn't subclass parts of RunLoop. wdyt? > Are you okay with the fact that the Delegate holds state that is really > meant to be used by RunLoop? > I'm okay with something holding state that is used by RunLoop. But I think that Delegate interface should just be an interface that RunLoop wants to call to do things. The other stuff used by the Delegate to implement itself could be separate as per above. That feels like a cleaner separation to me. > I agree it's a bit of a weird indirection > but it's the best ownership model I could think of. The state is really > all on the Delegate (only one per thread). > If it works and feels nicer, RunLoop could instead inherit the "Private" stuff above and give a raw pointer to itself to MessageLoop instead of mallocing a Private thing with access to RunLoop internals. > RunLoop instances are merely dedicated drivers of that state. > > As for the protected Delegate methods that allow MessageLoop to poke > back into that private RunLoop state which it owns but doesn't have > access to... IsNested() could be implemented MessageLoop side, it's just > that we already the information so poking at it directly seemed simpler. > GetTopMostRunLoop() will go away as I split this even further. > > https://codereview.chromium.org/2880453003/diff/20001/ > base/run_loop.h#newcode146 > base/run_loop.h:146: friend class RunLoop; > On 2017/05/15 16:23:13, danakj wrote: > > This isn't needed since the class is nested right? > > In Java you're right I think but in C++ it's required. > This compiles for instance: class Outer { public: Outer(int i) : i_(i) {} class Inner { public: Inner(Outer* o) { std::cout << o->i_ << '\n'; } }; private: int i_; }; int main() { Outer o(7); Outer::Inner i(&o); return 0; } -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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> wrote: > > > 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 wrote: > > > nit: "RunLoop::Delegate implementation." ? > > > > I've been told to do the opposite (this CL's version) many times and the > > paradigm is all over the codebase so I've now adopted it per less chars. > > > > https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h > > File base/run_loop.h (right): > > > > https://codereview.chromium.org/2880453003/diff/20001/ > > base/run_loop.h#newcode123 > > base/run_loop.h:123: class BASE_EXPORT Delegate { > > On 2017/05/15 16:23:13, danakj wrote: > > > While its less # of classes to do this, this Delegate is combining two > > things > > > into one which I feel is awkward: > > > 1. Operations that RunLoop wants to delegate to another class > > (Run/Quit). > > > 2. Private APIs for the delegate to use from RunLoop. > > > > > > IOW half the methods are for RunLoop to use and half are for the > > Delegate to > > > use. > > > > > > What if we split them and did something like > > > > > > class Delegate { > > > Init(Private*) > > > Run() = 0; > > > Quit() = 0; > > > } > > > > > > class Private { > > > GetTopMostRunLoop() { ... } > > > IsNested() { ... } > > > } > > > > > > etc.. > > > > > > This also I think gets us more like having interface (abstract APIs) > > separate > > > from implementations which has a better code smell to me. > > > > > > wdyt? > > > > Hmmm, I don't see how this would work, what is MessageLoop implementing? > > > > I guess we need a created instance per messageloop so itd be like > > class MessageLoop : public RunLoop::Delegate { > > // Delegate > Init(unique_ptr<RunLoop::Private>); > Run(); > Quit(); > > unique_ptr<RunLoop::Private> run_loop_stuff_; > } > > This way MessageLoop holds parts of RunLoop essentially, but doesn't > subclass parts of RunLoop. wdyt? Init(unique_ptr<RunLoop::Private>) takes ownership and saves it in Delegate's privates? That means every Delegate impl will have to call this and malloc a Private into its Delegate self? Then makes me feel like that code could be shared, i.e. all in the Delegate impl as it is right now... The current impl is basically that with the bits common to every Delegate impl squished into self. I do like the logic flow of your proposal though, I'm just not confident it's less confusing than the current CL. Also Delegate::Init(unique_ptr<RunLoop::Private>) would still be a call intended to happen MessageLoop->RunLoop so this doesn't get rid of the fact that methods go both ways... In the current impl you can see it this way: From RunLoop's point of view: * There is one RunLoop::Delegate per thread (at most). * RunLoop instances are driving it. From RunLoop::Delegate impls' point of view: * They must BindToCurrentThread() before RunLoop instances can be used on their thread. * They can reach into Delegate provided methods on self to gather required bits of intelligence in a const-way (once GetTopMostRunLoop() goes away). If we really want to split concerns I think the proposed Delegate::Init would need to be a static RunLoop::RegisterDelegateForCurrentThread(Delegate*) which itself creates the privates and attaches them to the instance... but that just seems like going out of our way to recreate the same runtime state... Overall I'm not sure it's a big concern? This isn't an API intended to be implemented by many... > > > > > Are you okay with the fact that the Delegate holds state that is really > > meant to be used by RunLoop? > > > > I'm okay with something holding state that is used by RunLoop. But I think > that Delegate interface should just be an interface that RunLoop wants to > call to do things. The other stuff used by the Delegate to implement itself > could be separate as per above. That feels like a cleaner separation to me. > > > > I agree it's a bit of a weird indirection > > but it's the best ownership model I could think of. The state is really > > all on the Delegate (only one per thread). > > > > If it works and feels nicer, RunLoop could instead inherit the "Private" > stuff above and give a raw pointer to itself to MessageLoop instead of > mallocing a Private thing with access to RunLoop internals. > > > > RunLoop instances are merely dedicated drivers of that state. > > > > As for the protected Delegate methods that allow MessageLoop to poke > > back into that private RunLoop state which it owns but doesn't have > > access to... IsNested() could be implemented MessageLoop side, it's just > > that we already the information so poking at it directly seemed simpler. > > GetTopMostRunLoop() will go away as I split this even further. > > > > https://codereview.chromium.org/2880453003/diff/20001/ > > base/run_loop.h#newcode146 > > base/run_loop.h:146: friend class RunLoop; > > On 2017/05/15 16:23:13, danakj wrote: > > > This isn't needed since the class is nested right? > > > > In Java you're right I think but in C++ it's required. > > > > This compiles for instance: > > class Outer { > public: > Outer(int i) : i_(i) {} > > class Inner { > public: > Inner(Outer* o) { std::cout << o->i_ << '\n'; } > }; > > private: > int i_; > }; > > > int main() > { > Outer o(7); > Outer::Inner i(&o); > return 0; > } Right, Inner can access Outer (with explicit pointer -- that's the difference with Java, my bad), but Outer can't access Inner without friend.
Description was changed from ========== 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 ========== to ========== 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.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 ==========
Description was changed from ========== 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.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 ========== to ========== 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 ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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 checked by gab@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) android_optional_gpu_tests_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/15 20:08:31, gab wrote: > 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> wrote: > > > > > 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 wrote: > > > > nit: "RunLoop::Delegate implementation." ? > > > > > > I've been told to do the opposite (this CL's version) many times and the > > > paradigm is all over the codebase so I've now adopted it per less chars. > > > > > > https://codereview.chromium.org/2880453003/diff/20001/base/run_loop.h > > > File base/run_loop.h (right): > > > > > > https://codereview.chromium.org/2880453003/diff/20001/ > > > base/run_loop.h#newcode123 > > > base/run_loop.h:123: class BASE_EXPORT Delegate { > > > On 2017/05/15 16:23:13, danakj wrote: > > > > While its less # of classes to do this, this Delegate is combining two > > > things > > > > into one which I feel is awkward: > > > > 1. Operations that RunLoop wants to delegate to another class > > > (Run/Quit). > > > > 2. Private APIs for the delegate to use from RunLoop. > > > > > > > > IOW half the methods are for RunLoop to use and half are for the > > > Delegate to > > > > use. > > > > > > > > What if we split them and did something like > > > > > > > > class Delegate { > > > > Init(Private*) > > > > Run() = 0; > > > > Quit() = 0; > > > > } > > > > > > > > class Private { > > > > GetTopMostRunLoop() { ... } > > > > IsNested() { ... } > > > > } > > > > > > > > etc.. > > > > > > > > This also I think gets us more like having interface (abstract APIs) > > > separate > > > > from implementations which has a better code smell to me. > > > > > > > > wdyt? > > > > > > Hmmm, I don't see how this would work, what is MessageLoop implementing? > > > > > > > I guess we need a created instance per messageloop so itd be like > > > > class MessageLoop : public RunLoop::Delegate { > > > > // Delegate > > Init(unique_ptr<RunLoop::Private>); > > Run(); > > Quit(); > > > > unique_ptr<RunLoop::Private> run_loop_stuff_; > > } > > > > This way MessageLoop holds parts of RunLoop essentially, but doesn't > > subclass parts of RunLoop. wdyt? > > Init(unique_ptr<RunLoop::Private>) takes ownership and saves it in Delegate's > privates? That means every Delegate impl will have to call this and malloc a > Private into its Delegate self? Then makes me feel like that code could be > shared, i.e. all in the Delegate impl as it is right now... > > The current impl is basically that with the bits common to every Delegate impl > squished into self. I agree, the difference is that you divide responsibilities between APIs, so there's not an API called Delegate for the RunLoop that is also a DelegateClient for the Delegate's impl (what the Delegate API is right now). > I do like the logic flow of your proposal though, I'm just not confident it's > less confusing than the current CL. If it helps, the part I find confusing is reading MessageLoop it now appears to be calling methods on itself when it's really calling methods on RunLoop, indirectly thru the RunLoop::Delegate. Whereas it would instead be clearly calls to some other object if it was not inheriting parts of (or access to) RunLoop, and instead owned a thing that provided it. thing_to_use_run_loop_->DoThing() as opposed to this->DoThing() essentially, where |this| is a MessageLoop but DoThing() is not part of MessageLoop and acts on RunLoop instead. Does this better explain why I don't like the pattern being constructed here? If you're not sold on this being awkward after reading this then we can go with the way you've done it. > Also Delegate::Init(unique_ptr<RunLoop::Private>) would still be a call intended to > happen MessageLoop->RunLoop so this doesn't get rid of the fact that methods go > both ways... I think this is not what I meant. Init() would be called from RunLoop on its Delegate to tell it about this helper that provides access to it, but I see what you mean below as I was missing the TLS part of this. > In the current impl you can see it this way: > > From RunLoop's point of view: > * There is one RunLoop::Delegate per thread (at most). > * RunLoop instances are driving it. > > From RunLoop::Delegate impls' point of view: > * They must BindToCurrentThread() before RunLoop instances can be used on their > thread. > * They can reach into Delegate provided methods on self to gather required bits > of intelligence in a const-way (once GetTopMostRunLoop() goes away). > > > If we really want to split concerns I think the proposed Delegate::Init would > need to be a static RunLoop::RegisterDelegateForCurrentThread(Delegate*) which > itself creates the privates and attaches them to the instance... but that just > seems like going out of our way to recreate the same runtime state... I see about the TLS stuff. I agree a static method would make sense, especially since you need to call it before making any RunLoops. Then RunLoop's constructor would call Init to provide the helpers. class MessageLoop : public RunLoop::Delegate { // Delegate: void Run() override; void Quit() override; void BindToCurrentThread() { ... MessageLoop things ... helper_ = RunLoop::RegisterDelegateForThread(this); } private: RunLoopPrivateStuff* helper_; } class RunLoop { static RunLoopPrivateStuff* RegisterDelegateForThread(RunLoop::Delegate*) { .. make the private helper thing and store in tls or return existing one .. } RunLoop() : delegate_(.. get from tls ..), helper_(.. get from tls ..) { } private: class RunLoopPrivateStuff { .. can use RunLoop things .. }; RunLoopPrivateStuff private_; } I think that's what we get, so we have 2 TLS pointers, one for the helper and one for the delegate. Does this overall sound any better? If not then we can keep them together. > > Overall I'm not sure it's a big concern? This isn't an API intended to be > implemented by many... > > > > > > > > > > Are you okay with the fact that the Delegate holds state that is really > > > meant to be used by RunLoop? > > > > > > > I'm okay with something holding state that is used by RunLoop. But I think > > that Delegate interface should just be an interface that RunLoop wants to > > call to do things. The other stuff used by the Delegate to implement itself > > could be separate as per above. That feels like a cleaner separation to me. > > > > > > > I agree it's a bit of a weird indirection > > > but it's the best ownership model I could think of. The state is really > > > all on the Delegate (only one per thread). > > > > > > > If it works and feels nicer, RunLoop could instead inherit the "Private" > > stuff above and give a raw pointer to itself to MessageLoop instead of > > mallocing a Private thing with access to RunLoop internals. > > > > > > > RunLoop instances are merely dedicated drivers of that state. > > > > > > As for the protected Delegate methods that allow MessageLoop to poke > > > back into that private RunLoop state which it owns but doesn't have > > > access to... IsNested() could be implemented MessageLoop side, it's just > > > that we already the information so poking at it directly seemed simpler. > > > GetTopMostRunLoop() will go away as I split this even further. > > > > > > https://codereview.chromium.org/2880453003/diff/20001/ > > > base/run_loop.h#newcode146 > > > base/run_loop.h:146: friend class RunLoop; > > > On 2017/05/15 16:23:13, danakj wrote: > > > > This isn't needed since the class is nested right? > > > > > > In Java you're right I think but in C++ it's required. > > > > > > > This compiles for instance: > > > > class Outer { > > public: > > Outer(int i) : i_(i) {} > > > > class Inner { > > public: > > Inner(Outer* o) { std::cout << o->i_ << '\n'; } > > }; > > > > private: > > int i_; > > }; > > > > > > int main() > > { > > Outer o(7); > > Outer::Inner i(&o); > > return 0; > > } > > Right, Inner can access Outer (with explicit pointer -- that's the difference > with Java, my bad), but Outer can't access Inner without friend. Oh, you're friending the opposite direction than I read that, my bad.
The CQ bit was checked by gab@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, managed to split the interfaces without introducing heap objects and while still using a single TLS slot :)
The CQ bit was checked by gab@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM :)
Description was changed from ========== 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 ========== to ========== 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=xingliu for network_listener_unittest.cc include fix ==========
gab@chromium.org changed reviewers: + xingliu@chromium.org
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2880453003/#ps140001 (title: "rebase on r472282 and fix new lacking message_loop.h")
TBR xingliu for network_listener_unittest.cc include fix
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== 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=xingliu for network_listener_unittest.cc include fix ========== to ========== 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 ==========
gab@chromium.org changed reviewers: + phajdan.jr@chromium.org
TBR=phajdan.jr@chromium.org for content\public\test\cache_test_util.h fwd-decl nit
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, xingliu@chromium.org Link to the patchset: https://codereview.chromium.org/2880453003/#ps160001 (title: "+missing fwd-decl in cache_test_util.h")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, xingliu@chromium.org Link to the patchset: https://codereview.chromium.org/2880453003/#ps180001 (title: "Allow IsRunning/IsNestedOnCurrentThread() without a registered Delegate")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, xingliu@chromium.org Link to the patchset: https://codereview.chromium.org/2880453003/#ps200001 (title: "oops, re-invert IsRunning logic")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gab@chromium.org
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": 200001, "attempt_start_ts": 1495079806868190, "parent_rev": "3577357065de1e5a173163335406faa28932c07c", "commit_rev": "27355196d32f75606b3e43b54bd0d03ef42b4579"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/27355196d32f75606b3e43b54bd0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/27355196d32f75606b3e43b54bd0... |