[iOS tracing] Move StartupTracingController to components/tracing/common [chromium/src : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
I don't love the SetAndroidPathGeneratorCallback and SetIoTaskRunner hack, but otherwise it's a straighforward move. wdyt?
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
Mostly LG
component("startup_tracing") {If there isn't a need for this to be separate from "tracing_config" above, I'd lean towards folding it into the same target for simplicity (I think conceptually that target covers startup_tracing_controller; funnily "tracing_config" used to be called something like "startup_tracing").
#if BUILDFLAG(IS_ANDROID)
tracing::StartupTracingController::GetInstance()
.SetAndroidPathGeneratorCallback(base::BindRepeating(
&content::TracingControllerAndroid::GenerateTracingFilePath));
#endif
tracing::StartupTracingController::GetInstance().SetIoTaskRunner(
content::GetIOThreadTaskRunner({}));
tracing::StartupTracingController::GetInstance().StartIfNeeded();I've been slowly moving several tracing classes to RAII to avoid this awkward GetInstance() + SetSomething() pattern, and pass argument to constructor directly. (see e.g. TracingController)
WDYT of doing this to StartupTracingController:
- StartupTracingController takes these in constructor
- store StartupTracingController in BrowserMainLoop and BrowserTestBase (need to be accessible in StartupTracingTest)
- Call ShutdownAndWaitForStopIfNeeded in BrowserMainLoop::PreShutdown
- Remove GetInstance()
Related details
- Justin Cohen
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. |
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
component("startup_tracing") {If there isn't a need for this to be separate from "tracing_config" above, I'd lean towards folding it into the same target for simplicity (I think conceptually that target covers startup_tracing_controller; funnily "tracing_config" used to be called something like "startup_tracing").
This was to avoid
ERROR Dependency cycle:
//services/tracing/public/cpp:cpp ->
//components/tracing:tracing_config ->
//services/tracing/public/cpp:cpp
#if BUILDFLAG(IS_ANDROID)
tracing::StartupTracingController::GetInstance()
.SetAndroidPathGeneratorCallback(base::BindRepeating(
&content::TracingControllerAndroid::GenerateTracingFilePath));
#endif
tracing::StartupTracingController::GetInstance().SetIoTaskRunner(
content::GetIOThreadTaskRunner({}));
tracing::StartupTracingController::GetInstance().StartIfNeeded();I've been slowly moving several tracing classes to RAII to avoid this awkward GetInstance() + SetSomething() pattern, and pass argument to constructor directly. (see e.g. TracingController)
WDYT of doing this to StartupTracingController:
- StartupTracingController takes these in constructor
- store StartupTracingController in BrowserMainLoop and BrowserTestBase (need to be accessible in StartupTracingTest)
- Call ShutdownAndWaitForStopIfNeeded in BrowserMainLoop::PreShutdown
- Remove GetInstance()
Sure. I'm still quite new to this area of code but I think this is what you had in mind. PTAL!
Related details
- 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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
[iOS tracing] Move StartupTracingController to components/tracing/commonI will defer to components/tracing/OWNERS (e.g. @etie...@chromium.org)
Please close this to add visibility again, and I will stamp after a sanity check.
Bug: 495937056
```
Bug: 495937056Nit: Duplicate.
Related details
- Etienne Pierre-Doray
- Justin Cohen
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. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Bug: 495937056
```
Bug: 495937056Justin CohenNit: Duplicate.
Done
Related details
- Arthur Sonzogni
- 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")
component("startup_tracing") {Justin CohenIf there isn't a need for this to be separate from "tracing_config" above, I'd lean towards folding it into the same target for simplicity (I think conceptually that target covers startup_tracing_controller; funnily "tracing_config" used to be called something like "startup_tracing").
This was to avoid
ERROR Dependency cycle:
//services/tracing/public/cpp:cpp ->
//components/tracing:tracing_config ->
//services/tracing/public/cpp:cpp
Ah I see!
Come to think of it, any reason we can't put startup_tracing_controller.h directly in services/tracing/public/cpp?
class EmergencyTraceFinalisationCoordinator {
public:
static EmergencyTraceFinalisationCoordinator& GetInstance() {
static base::NoDestructor<EmergencyTraceFinalisationCoordinator> instance;
return *instance;
}I don't like the lifetime of EmergencyTraceFinalisationCoordinator (and the design in general), and saw it's making things more complicated in the follow-up.
I tried something here: https://chromium-review.git.corp.google.com/c/chromium/src/+/7779464
To delete EmergencyTraceFinalisationCoordinator entirely and manage all lifetime in StartupTracingController.
Feel free to incorporate in your CL.
if (startup_tracing_controller_) {
startup_tracing_controller_->ShutdownAndWaitForStopIfNeeded();
}We might want to move this at the end of the function so the move is a no-op.
#if BUILDFLAG(IS_ANDROID)
tracing::StartupTracingController::GetInstance()
.SetAndroidPathGeneratorCallback(base::BindRepeating(
&content::TracingControllerAndroid::GenerateTracingFilePath));
#endif
tracing::StartupTracingController::GetInstance().SetIoTaskRunner(
content::GetIOThreadTaskRunner({}));
tracing::StartupTracingController::GetInstance().StartIfNeeded();Justin CohenI've been slowly moving several tracing classes to RAII to avoid this awkward GetInstance() + SetSomething() pattern, and pass argument to constructor directly. (see e.g. TracingController)
WDYT of doing this to StartupTracingController:
- StartupTracingController takes these in constructor
- store StartupTracingController in BrowserMainLoop and BrowserTestBase (need to be accessible in StartupTracingTest)
- Call ShutdownAndWaitForStopIfNeeded in BrowserMainLoop::PreShutdown
- Remove GetInstance()
Sure. I'm still quite new to this area of code but I think this is what you had in mind. PTAL!
Yes that works!
I didn't realize this would mean a few member function (SetDefaultBasename) need to become static for testing, but I guess that's ok.
Related details
- Arthur Sonzogni
- Justin Cohen
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. |