arc: enable Android tracing from chrome://tracing in dev mode

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 5fc480346a29332414bbd150039dfcabea0385ee..0f272991579a256538c08b766dd0eb657840e81f 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -3,6 +3,10 @@
 // found in the LICENSE file.
 #include "content/browser/tracing/tracing_controller_impl.h"
 
+#include <algorithm>
+#include <memory>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/cpu.h"
 #include "base/files/file_util.h"
@@ -45,6 +49,7 @@
 #if defined(OS_CHROMEOS)
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
+#include "chromeos/trace/arc_trace_agent.h"
 #endif
 
 #if defined(OS_WIN)
@@ -63,6 +68,7 @@ base::LazyInstance<TracingControllerImpl>::Leaky g_controller =
 
 const char kChromeTracingAgentName[] = "chrome";
 const char kETWTracingAgentName[] = "etw";
+const char kArcTracingAgentName[] = "arc";
 const char kChromeTraceLabel[] = "traceEvents";
 
 const int kStartTracingTimeoutSeconds = 30;
@@ -289,6 +295,12 @@ bool TracingControllerImpl::StartTracing(
                      base::Unretained(this)));
       ++pending_start_tracing_ack_count_;
     }
+
+    chromeos::ArcTraceAgent::GetInstance()->StartAgentTracing(
+        trace_config,
+        base::Bind(&TracingControllerImpl::OnStartAgentTracingAcked,
+                   base::Unretained(this)));
+    ++pending_start_tracing_ack_count_;
 #elif defined(OS_WIN)
     EtwTracingAgent::GetInstance()->StartAgentTracing(
         trace_config,
@@ -563,6 +575,12 @@ void TracingControllerImpl::AddTracingAgent(const std::string& agent_name) {
         BrowserThread::GetBlockingPool());
     return;
   }
+
+  auto* arc_trace_agent = chromeos::ArcTraceAgent::GetInstance();

[Daniel Erat, 2017/03/08 02:58:40]

this pattern feels a bit weird to me. it looks like we always instantiate this
singleton, even if arc isn't enabled. the tracing agent name is static, and i
think it's also duplicated in the kArcTracingAgentName constant declared above.
why does GetTracingAgentName() need to be called here?

[Earl Ou, 2017/03/08 05:03:04]

We always instantiate this as we don't know if there is arc or not (otherwise we
need dependency on /components/arc or /chrome/browser/chromeos/arc).

For calling GetTracingAgentName(), true that it's weird, but all other tracing
agent applies the same pattern here, e.g., debug_deamon, etw_agent, power_agent,
so I just keep it consistent.

[Daniel Erat, 2017/03/08 05:21:07]

hmm. i'm not an expert on tracing, but does this method only get called with an
agent_name of "arc" if ARC tracing is explicitly requested? i'm just wondering
why this pattern exists, and why this method doesn't instead use the string
constants defined within this file to avoid instantiating agents unnecessarily.

[Earl Ou, 2017/03/08 07:11:10]

This function is called in the callback of StartTracingAgent, so indeed it's
called with 'arc' if we previously call ArcTracingAgent::StartTracingAgent(...).

I think we can just use the string const to avoid instantiating here. The reason
is just for consistency, as other trace agents might not have the similar
constant in this file.

[Earl Ou, 2017/03/08 07:59:28]

I changed this to use the constant instead of instantiating instance.

+  if (agent_name == arc_trace_agent->GetTracingAgentName()) {
+    additional_tracing_agents_.push_back(arc_trace_agent);
+    return;
+  }
 #elif defined(OS_WIN)
   auto* etw_agent = EtwTracingAgent::GetInstance();
   if (agent_name == etw_agent->GetTracingAgentName()) {
@@ -667,15 +685,17 @@ void TracingControllerImpl::OnEndAgentTracingAcked(
 
   if (trace_data_sink_.get() && events_str_ptr &&
       !events_str_ptr->data().empty()) {
-    std::string json_string;
     if (agent_name == kETWTracingAgentName) {
       // The Windows kernel events are kept into a JSON format stored as string
       // and must not be escaped.
-      json_string = events_str_ptr->data();
-    } else {
-      json_string = base::GetQuotedJSONString(events_str_ptr->data());
+      trace_data_sink_->AddAgentTrace(events_label, events_str_ptr->data());
+    } else if (agent_name != kArcTracingAgentName) {
+      // ARC trace data is obtained via systrace. Ignore the empty data.
+      // For other trace data, quoted as JSON string and merge them into

[Daniel Erat, 2017/03/07 15:53:22]

nit: "Quote other trace data as JSON strings and merge them into ..."

[Earl Ou, 2017/03/08 05:03:04]

Done.

+      // |trace_data_sink|.

[Daniel Erat, 2017/03/07 15:53:22]

nit: |trace_data_sink_| (with trailing underscore)

[Earl Ou, 2017/03/08 05:03:04]

Done.

+      trace_data_sink_->AddAgentTrace(
+          events_label, base::GetQuotedJSONString(events_str_ptr->data()));
     }
-    trace_data_sink_->AddAgentTrace(events_label, json_string);
   }
   std::vector<std::string> category_groups;
   OnStopTracingAcked(NULL, category_groups);
@@ -830,7 +850,7 @@ void TracingControllerImpl::RecordClockSyncMarker(
 
 void TracingControllerImpl::IssueClockSyncMarker() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(pending_clock_sync_ack_count_ == 0);
+  DCHECK_EQ(0, pending_clock_sync_ack_count_);
 
   for (auto* it : additional_tracing_agents_) {
     if (it->SupportsExplicitClockSync()) {
@@ -870,7 +890,7 @@ void TracingControllerImpl::OnClockSyncMarkerRecordedByAgent(
     return;
 
   // Stop tracing only if all agents report back.
-  if(--pending_clock_sync_ack_count_ == 0) {
+  if (--pending_clock_sync_ack_count_ == 0) {
     clock_sync_timer_.Stop();
     StopTracingAfterClockSync();
   }