Implement system tracing for cros

content/browser/tracing/tracing_controller_impl.cc

Index: content/browser/tracing/tracing_controller_impl.cc
diff --git a/content/browser/tracing/tracing_controller_impl.cc b/content/browser/tracing/tracing_controller_impl.cc
index 4c08eacfd90281c0e4af8466a62776462c3a0e8e..d1184964fe750868da9cb162897cc4bfed9598f3 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -15,6 +15,11 @@
 #include "content/public/browser/browser_message_filter.h"
 #include "content/public/common/content_switches.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/debug_daemon_client.h"
+#endif
+
 using base::debug::TraceLog;
 
 namespace content {
@@ -120,7 +125,8 @@ TracingControllerImpl::TracingControllerImpl() :
     // Tracing may have been enabled by ContentMainRunner if kTraceStartup
     // is specified in command line.
     is_recording_(TraceLog::GetInstance()->IsEnabled()),
-    is_monitoring_(false) {
+    is_monitoring_(false),
+    is_system_tracing_(false) {
 }
 
 TracingControllerImpl::~TracingControllerImpl() {
@@ -194,7 +200,14 @@ bool TracingControllerImpl::EnableRecording(
   if (options & ENABLE_SAMPLING) {
     trace_options |=  TraceLog::ENABLE_SAMPLING;
   }
-  // TODO(haraken): How to handle ENABLE_SYSTRACE?
+  if (options & ENABLE_SYSTRACE) {

[dsinclair, 2014/01/30 14:36:06]

Is SYSTRACE only for cros, or will this if () also be used for Android? If just
CrOS, can we put the #define outside the if () so the branch gets pre-processed
out?

[haraken, 2014/01/31 05:58:27]

Done.

+#if defined(OS_CHROMEOS)
+    DCHECK(!is_system_tracing_);
+    chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->
+      StartSystemTracing();

[dsinclair, 2014/01/30 14:36:06]

nit: indenting.

[haraken, 2014/01/31 05:58:27]

I'm not sure but this is in the middle of an expression (not parameters of a
function), so it should be 2-space indented. The old code also used 2-space
indentation.

+    is_system_tracing_ = true;
+#endif
+  }
 
   base::Closure on_enable_recording_done_callback =
       base::Bind(&TracingControllerImpl::OnEnableRecordingDone,
@@ -627,6 +640,23 @@ void TracingControllerImpl::OnDisableRecordingAcked(
 void TracingControllerImpl::OnResultFileClosed() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (is_system_tracing_) {
+    // Disable system tracing now that the local trace has shutdown.
+    // This must be done last because we potentially need to push event
+    // records into the system event log for synchronizing system event
+    // timestamps with chrome event timestamps--and since the system event
+    // log is a ring-buffer (on linux) adding them at the end is the only
+    // way we're confident we'll have them in the final result.
+    is_system_tracing_ = false;

[dsinclair, 2014/01/30 14:36:06]

This can only be set true inside a #if defined(OS_CHROMEOS) so can we move this
whole block inside the #if define below (including the outer if
(is_system_tracing_)?

[haraken, 2014/01/31 05:58:27]

Done.

+#if defined(OS_CHROMEOS)
+    chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->
+      RequestStopSystemTracing(

[dsinclair, 2014/01/30 14:36:06]

nit: indenting

+          base::Bind(&TracingMessageHandler::OnEndSystemTracingAcked,
+                     base::Unretained(this)));
+#endif
+    return;
+  }
+
   if (!result_file_)
     return;
 
@@ -637,6 +667,16 @@ void TracingControllerImpl::OnResultFileClosed() {
   result_file_.reset();
 }
 
+void TracingControllerImpl::OnEndSystemTracingAcked(
+    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  TracingUI* tracing_ui = 0;

[haraken, 2014/01/30 13:39:46]

This is a problem. Since TracingController does not know which TracingUI called
TracingController::DisableRecording, TracingController cannot dispatch
OnSystemTraceDataCollected to a proper TracingUI.

Since TracingController::DisableRecording is called by static methods of
TracingUI, it is not trivial to teach TracingController which TracingUI called
TracingController::DisableRecording.

(Note: TracingController knows a list of existing TracingUIs. They are stored in
TracingControllerImpl::tracing_uis_.)

How can we address this issue?

[dsinclair, 2014/01/30 14:36:06]

So, does tracing_ui matter in this case? I though we wanted to dump the system
trace data into the file the same as we're dumping in regular trace data? I
guess the problem we have is that the JSON objects in the file are already
complete and we have no way to inject the extra systrace data into them correct?

[haraken, 2014/01/30 15:25:02]

I'm not familiar with how the system tracing was working, so I want to
understand :)

If we can just append system tracing data to the same JSON file, I think I can
implement that way.

I thought we need to call "tracingController.onSystemTraceDataCollected" since
the previous tracing_ui.cc was doing that (Line 553 of
https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...).

[haraken, 2014/01/31 05:58:27]

Done. I changed the code so that the system tracing data is appended to
|result_file_|.

+  tracing_ui->OnSystemTraceDataCollected(events_str_ptr);
+  DCHECK(!is_system_tracing_);
+  OnResultFileClosed();
+}
+
 void TracingControllerImpl::OnCaptureMonitoringSnapshotAcked(
     TraceMessageFilter* trace_message_filter) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {