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

chromeos/trace/arc_trace_agent.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_TRACE_ARC_TRACE_AGENT_H_
+#define CHROMEOS_TRACE_ARC_TRACE_AGENT_H_
+
+#include "base/callback_forward.h"
+#include "base/trace_event/trace_event.h"
+#include "base/trace_event/tracing_agent.h"
+#include "chromeos/chromeos_export.h"
+
+namespace chromeos {
+
+// A wrapper for controlling tracing in ARC container.
+// The overriden Stop/StartAgentTracing functions are implemented by the
+// Delegate object to interact with ARC bridge. All the functions are expected
+// to be called on the brower's UI thread.
+class CHROMEOS_EXPORT ArcTraceAgent : public base::trace_event::TracingAgent {

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

this should probably be named ArcTracingAgent instead to match the interface
name and other implementations.

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

Done.

+ public:
+  class Delegate {
+   public:
+    using StartTracingCallback = base::Callback<void(bool)>;

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

document parameters

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

Done.

+    using StopTracingCallback = base::Callback<void(bool)>;
+
+    virtual ~Delegate();
+
+    virtual void StartTracing(

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

please add method-level comments documenting what these methods do

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

Done.

+        const base::trace_event::TraceConfig& trace_config,
+        const StartTracingCallback& callback) = 0;
+
+    virtual void StopTracing(const StopTracingCallback& callback) = 0;
+  };
+
+  virtual ~ArcTraceAgent();

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

use 'override' instead of 'virtual' here, i think

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

We actually implement this class in a subclass, and use pointer ArcTraceAgent*
to own the subclass. So we should need 'virtual' here?

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

i think it should have 'override' since it's overriding TracingAgent's d'tor.

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

You're right. I need to take some c++ course :)

+
+  static ArcTraceAgent* GetInstance();

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

does this actually need to be a singleton? if not, clear ownership is better.

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

This is related to the dependency issue. The instance should be owned by
ArcService, but the dependency between content/ and arc is not allowed. That's
why we're using singleton here.

+
+  // Sets Delegate instance, which should implement the communication
+  // using ARC bridge. If set to nullptr, calling of Start/StopAgentTracing
+  // does nothing.
+  virtual void SetDelegate(Delegate* delegate) = 0;

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

if ownership of |delegate| is transferred, it should be  a std::unique_ptr. if
it isn't, please document that

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

It's owned by ArcServiceLauncher, I added comments here.

+};

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

DISALLOW_COPY_AND_ASSIGN

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

Done.

+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_TRACE_ARC_TRACE_AGENT_H_