[task] Use message pump in InProcessRendererThread [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 6 comments![]( "Open in Gerrit")
This removes the only usage of ThreadControllerImpl in prod(-ish). ThisEtienne Pierre-DorayI don't see any ThreadControllerImpl usage changing in this CL? Is this now just the precursor API CL?
Gabriel CharetteThis is removing the implicit creation of ThreadControllerImpl here
https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/in_process_renderer_thread.cc;l=44?q=InProcessRendererThread::Init&ss=chromiumBut I guess it only becomes obvious in the follow up where this is made more explicit in CreateMainThreadSchedulerForTesting()
Etienne Pierre-DorayI see, let's clarify in CL description?
Done
bool restartable = false,Etienne Pierre-DorayI'm a big fan for using enum classes for things like this one :).
Acknowledged (done in precursor)
explicit Thread(const std::string& name,Etienne Pierre-DorayCould we add a comment here about |restartable| and |delegate| being mutually exclusive (and ideally why)?
(also some description of what |restartable| is and why it's needed in general would be welcome)
Acknowledged (done in precursor)
false,Etienne Pierre-Doraynit: /*restartable=*/ (or enum class).
Acknowledged
base::MessagePump::Create(base::MessagePumpType::DEFAULT));Etienne Pierre-DorayLet's `DCHECK(base::ThreadTaskRunnerHandle::IsSet());` and `DCHECK(base::CurrentThread::IsSet());` after this call with a comment that says:
// Make sure the contract of Thread::Delegate::BindToCurrentThread() is fulfilled.
(it's non-obvious to me from this that it is though I can see how deep down this call it's probably happening; saw some code but also some conditionals...)
Done
Related details
- Alexander Timin
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gabriel Charette
| Code-Review | +1 |
3 comments
This removes the only usage of ThreadControllerImpl in prod(-ish)Let's clarify that this is by removing the last call to CreateMainThreadScheduler w/o a MessagePump?
.SetShouldReportLockMetrics(is_main_thread)Why this change? Explain in CL desc why that's related?
Related details
- Alexander Timin
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
+dcheng@ for content/renderer/
This removes the only usage of ThreadControllerImpl in prod(-ish)Let's clarify that this is by removing the last call to CreateMainThreadScheduler w/o a MessagePump?
Done
void InProcessRendererThread::ThreadDelegate::BindToCurrentThread(Etienne Pierre-DoraySeems this should still call the underlying Delegate::BindToCurrentThread()?
But overall I don't see why we need to do this here instead of in Init() I'm surprised BindToCurrentThread us overridable at all, seems like a second way of handling Init? Document the difference if it is indeed needed?
Etienne Pierre-DorayThis overrides a pure virtual class (we completely bypass the default SequenceManagerThreadDelegate)
From base::Thread impl, BindToCurrentThread is called earlier and is expected to create the scheduling environment (DCHECK(ThreadTaskRunnerHandle::IsSet()))
We're using this to create our own SequenceManager (via CreateMainThreadScheduler); doing it in Init would be too late.https://source.chromium.org/chromium/chromium/src/+/main:base/threading/thread.cc;l=370;bpv=1;bpt=1
Acknowledged
Why this change? Explain in CL desc why that's related?
- Alexander Timin
- Daniel Cheng
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |