ios: Enable native Perfetto data sources for developer profiling [chromium/src : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
For discussion -- a first draft that's maybe landable? WDYT?
Related details
- Etienne Pierre-Doray
- Justin Novosad
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 4 comments![]( "Open in Gerrit")
Publishing my initial comments
class IOSBackgroundTracingManager {Looking at what this does, it fills a role closer to StartupTracingController than BackgroundTracingManager (counterpart in content) + some initialization logic.
In fact, I'm wondering how much of StartupTracingController we could simply more to services and reuse here, which would support the same set of command line arguments.
void IOSBackgroundTracingManager::Initialize() {Even though you might not need all of PerfettoTracedProcess, I think we can improve code sharing (by extracting and reusing parts of it)
- I think we want to use base::tracing::PerfettoPlatform on iOS
- We should reuse the same code for DataSource registrations
desc.set_name(tracing::mojom::kHistogramSampleSourceName);There's no reason for these to be in mojo anymore, I think we should just move the declarations in a plain header in services/tracing/public/cpp/perfetto/
And then this can unconditionally refer to it.
Related details
- Justin Cohen
- Justin Novosad
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 Novosad (Gerrit)
Justin Novosad added 8 comments![]( "Open in Gerrit")
startButton.disabled = false;The start button should only be re-enbaled once trace processing is complete. I.e. at the same time that the statusText becomes "Ready".
stopButton.disabled = true;Disabling the stop button should probably be done earlier, in the stopButton's event listener. Reduces the potential for a race condition.
}, 2000);This 2 second delay is wrong. There should be another callback that triggers this transition upon completion of the trace processing.
[ChromeEarlGrey waitForWebStateContainingText:"Stop and Share"];Isn't this condition tautological? I mean the button with this label remains visible regardless of whether or not the button is disabled. This test should instead observe UI state transitions by monitoring the statusText element. In this case, I think it should be waiting for the text to change to "Recording...".
[ChromeEarlGrey waitForWebStateContainingText:"Start Recording"];Same issue here. The test should be waiting for the statusText to change back to "Ready" IMHO.
#include "components/variations/active_field_trials.h" // nogncheckWhy "nogncheck" here? Either fix the gn file or provide an explanation for why an exception is absolutely required.
#include "services/tracing/public/mojom/perfetto_service.mojom.h" // nognchecknogncheck?
#include "services/tracing/public/mojom/perfetto_service.mojom.h" // nognchecksame here.
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. |