[Tracing Clock Sync] Add TracingAgent interface in Chrome

[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
Committed: https://crrev.com/b0b2a7f0fd7f6ec4a43f735e643a1f2f90351dc8
Cr-Commit-Position: refs/heads/master@{#365284}

[Zhen Wang, 2015-11-24 00:04:42]

Description was changed from

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

BUG=542390
==========

to

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks
corresponding subclasses.

Design doc:
https://goo.gl/8Vy6qX#heading=h.a4edivjnih9h

BUG=542390
==========

[Zhen Wang, 2015-11-24 00:06:39]

Description was changed from

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks
corresponding subclasses.

Design doc:
https://goo.gl/8Vy6qX#heading=h.a4edivjnih9h

BUG=542390
==========

to

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc:
https://goo.gl/8Vy6qX#heading=h.a4edivjnih9h

BUG=542390
==========

[Zhen Wang, 2015-11-24 03:49:23]

zhenw@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Zhen Wang, 2015-11-24 03:49:23]

ptal

[dsinclair, 2015-11-24 14:27:48]

dsinclair@chromium.org changed reviewers:
+ oysteine@chromium.org, simonhatch@chromium.org

[oystein (OOO til 10th of July), 2015-11-24 22:52:10]

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
base/trace_event/tracing_agent.h:16: 
Can you include some class-level comments here, clarifying what a TracingAgent
is?

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
base/trace_event/tracing_agent.h:46: static const char
kChromeTracingAgentName[];
Hmm why are these hardcoded here? It doesn't seem right that base/ should have
knowledge of this. Can the agents just return the string constants directly?

https://codereview.chromium.org/1468173003/diff/20001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/20001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:66: void
FakeDebugDaemonClient::IssueClockSyncMarker() {
The pattern of these three above functions seem to be the default, and repeated
a lot. Maybe this should be the default behavior in the base class, to reduce
the boilerplate?

[Zhen Wang, 2015-11-25 04:24:55]

How's the overall structure?

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
base/trace_event/tracing_agent.h:16: 
On 2015/11/24 22:52:10, Oystein wrote:
> Can you include some class-level comments here, clarifying what a TracingAgent
> is?

Done.

https://codereview.chromium.org/1468173003/diff/20001/base/trace_event/tracin...
base/trace_event/tracing_agent.h:46: static const char
kChromeTracingAgentName[];
On 2015/11/24 22:52:10, Oystein wrote:
> Hmm why are these hardcoded here? It doesn't seem right that base/ should have
> knowledge of this. Can the agents just return the string constants directly?

I originally thought about returning those strings directly. But I think it is
beneficial to have a centralized place to define those strings, so future
tracing agents know that they should use other names. Is there a better way of
doing that?

https://codereview.chromium.org/1468173003/diff/20001/chromeos/dbus/fake_debu...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/20001/chromeos/dbus/fake_debu...
chromeos/dbus/fake_debug_daemon_client.cc:66: void
FakeDebugDaemonClient::IssueClockSyncMarker() {
On 2015/11/24 22:52:10, Oystein wrote:
> The pattern of these three above functions seem to be the default, and
repeated
> a lot. Maybe this should be the default behavior in the base class, to reduce
> the boilerplate?

Done.

[shatch, 2015-11-25 16:49:31]

On 2015/11/24 03:49:23, Zhen Wang wrote:
> ptal

High level comments:

Really like the TracingAgents, maybe we can even push that a bit further and
cleanup the internals of TracingControllerImpl even more? Right now they all
derive from TracingAgent, so would it make sense for TracingController just to
hold a list of agents. Cuts down on some of the boilerplate for
starting/stopping/clock syncs, and also we have to do some book-keeping like
keeping track of pending_stop_tracing_acks every time one is added.


I'm not super clear what we gain by making TracingControllerImpl an agent as
well, is it simply to expose the clock sync stuff?

[Zhen Wang, 2015-11-26 01:20:07]

> Really like the TracingAgents, maybe we can even push that a bit further and
> cleanup the internals of TracingControllerImpl even more? Right now they all
> derive from TracingAgent, so would it make sense for TracingController just to
> hold a list of agents. Cuts down on some of the boilerplate for
> starting/stopping/clock syncs, and also we have to do some book-keeping like
> keeping track of pending_stop_tracing_acks every time one is added.

I updated the CL so that TracingControllerImpl holds a list of agents and their
starting/stopping is more unified. ptal.

[shatch, 2015-11-27 19:29:19]

I think I'm mostly fine with this, just a few comments.

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:148:
additional_tracing_agents_(
nit: Don't think you need these

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:302: for (it =
additional_tracing_agents_.begin();
for (auto it : additional_tracing_agents) ?

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl_data_sinks.cc (right):

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:141: if (it.key()
== base::trace_event::TracingAgent::kCrOSTracingAgentName ||
Don't need to do this now but could probably go further in the future and not
have these if statements, just have the agent supply the key name.

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
content/public/browser/tracing_controller.h:42: virtual void
AddAdditionalTracingAgentTrace(const std::string& agent_name,
nit: Long name, maybe just AddAgentTrace?

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
content/public/browser/tracing_controller.h:59: base::DictionaryValue
additional_tracing_agent_trace_;
Why not just a std::map?

[Zhen Wang, 2015-12-01 00:14:32]

Is it fine to declare and define static constants in TracingAgent? The intention
is to have a centralized place to define those. Are there better ways or they
are ok?

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:148:
additional_tracing_agents_(
On 2015/11/27 19:29:18, shatch wrote:
> nit: Don't think you need these

Done.

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:302: for (it =
additional_tracing_agents_.begin();
On 2015/11/27 19:29:18, shatch wrote:
> for (auto it : additional_tracing_agents) ?

Done.

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl_data_sinks.cc (right):

https://codereview.chromium.org/1468173003/diff/100001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl_data_sinks.cc:141: if (it.key()
== base::trace_event::TracingAgent::kCrOSTracingAgentName ||
On 2015/11/27 19:29:18, shatch wrote:
> Don't need to do this now but could probably go further in the future and not
> have these if statements, just have the agent supply the key name.

Done.

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
content/public/browser/tracing_controller.h:42: virtual void
AddAdditionalTracingAgentTrace(const std::string& agent_name,
On 2015/11/27 19:29:18, shatch wrote:
> nit: Long name, maybe just AddAgentTrace?

Done.

https://codereview.chromium.org/1468173003/diff/100001/content/public/browser...
content/public/browser/tracing_controller.h:59: base::DictionaryValue
additional_tracing_agent_trace_;
On 2015/11/27 19:29:18, shatch wrote:
> Why not just a std::map?

Done.

[Primiano Tucci (use gerrit), 2015-12-01 11:42:42]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-12-01 11:42:43]

Some drive-by nits

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:11: #include "base/trace_event/trace_config.h"
this header is not needed, just forward declare when possible (like in the case
of TraceConfig)

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:27: typedef base::Callback<void(
these days "using" is the thing :)
using StopAgentTracingCallback = base::Callback ...

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:54: scoped_ptr<base::DictionaryValue> marker);
Not related with this CL but out curiosity why this is not a TracedValue?

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:62: // Declare all possible tracing agent names
here. All new tracing agents
See https://google.github.io/styleguide/cppguide.html#Declaration_Order

- Move these constants up
- Then Ctors and dtor
- then the rest

https://codereview.chromium.org/1468173003/diff/120001/content/browser/tracin...
File content/browser/tracing/power_tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/120001/content/browser/tracin...
content/browser/tracing/power_tracing_agent.h:26: std::string
GetTracingAgentName() override;
Typically you want to add a comment like:

// base::TracingAgent implementation.

[Zhen Wang, 2015-12-01 18:02:53]

Thanks for the review, Primiano! :)

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:11: #include "base/trace_event/trace_config.h"
On 2015/12/01 11:42:43, Primiano Tucci wrote:
> this header is not needed, just forward declare when possible (like in the
case
> of TraceConfig)

Done.

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:27: typedef base::Callback<void(
On 2015/12/01 11:42:43, Primiano Tucci wrote:
> these days "using" is the thing :)
> using StopAgentTracingCallback = base::Callback ...

Done.

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:54: scoped_ptr<base::DictionaryValue> marker);
On 2015/12/01 11:42:43, Primiano Tucci wrote:
> Not related with this CL but out curiosity why this is not a TracedValue?

Because this marker will be recorded by the agent, which is not necessarily
Chrome (e.g., could be BattOr power monitor).

https://codereview.chromium.org/1468173003/diff/120001/base/trace_event/traci...
base/trace_event/tracing_agent.h:62: // Declare all possible tracing agent names
here. All new tracing agents
On 2015/12/01 11:42:43, Primiano Tucci wrote:
> See https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
> - Move these constants up
> - Then Ctors and dtor
> - then the rest

Done.

https://codereview.chromium.org/1468173003/diff/120001/content/browser/tracin...
File content/browser/tracing/power_tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/120001/content/browser/tracin...
content/browser/tracing/power_tracing_agent.h:26: std::string
GetTracingAgentName() override;
On 2015/12/01 11:42:43, Primiano Tucci wrote:
> Typically you want to add a comment like:
> 
> // base::TracingAgent implementation.

Done.

[oystein (OOO til 10th of July), 2015-12-01 19:31:10]

On 2015/12/01 00:14:32, Zhen Wang wrote:
> Is it fine to declare and define static constants in TracingAgent? The
intention
> is to have a centralized place to define those. Are there better ways or they
> are ok?

I personally think the base classes shouldn't know about the implementations in
higher layers, including any central list of constant names. If it's important
that the names are unique across the board, make that a constraint of the API
and just add a comment about it to tracing_agent.h.

It's not something I feel super strongly about though, if everyone else are fine
with it.

[shatch, 2015-12-01 19:47:34]

On 2015/12/01 19:31:10, Oystein wrote:
> On 2015/12/01 00:14:32, Zhen Wang wrote:
> > Is it fine to declare and define static constants in TracingAgent? The
> intention
> > is to have a centralized place to define those. Are there better ways or
they
> > are ok?
> 
> I personally think the base classes shouldn't know about the implementations
in
> higher layers, including any central list of constant names. If it's important
> that the names are unique across the board, make that a constraint of the API
> and just add a comment about it to tracing_agent.h.
> 
> It's not something I feel super strongly about though, if everyone else are
fine
> with it.

Yeah kinda sucks that adding a new agent in content or chromeos, you'd need to
modify base/trace_event/tracing_agent.h. Are there any other examples of this
kind of pattern in chrome?

[Zhen Wang, 2015-12-02 07:45:44]

Now removed the static constants. ptal.

[shatch, 2015-12-02 14:41:22]

lgtm from me, wait to see if Oystein or Primiano have more comments

[oystein (OOO til 10th of July), 2015-12-02 18:42:31]

lgtm, thank you!

[Zhen Wang, 2015-12-02 19:10:54]

+stevenjb for //chromeos/dbus/
+sievers for //content/public
+brettw for //tools/gn/bootstrap/bootstrap.py

[Zhen Wang, 2015-12-08 23:23:49]

Description was changed from

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc:
https://goo.gl/8Vy6qX#heading=h.a4edivjnih9h

BUG=542390
==========

to

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

[Zhen Wang, 2015-12-09 00:16:39]

Hi Simon and Oystein, can you take a look at the CL again? Now
IssueClockSyncMarker is removed.

[Zhen Wang, 2015-12-10 18:30:46]

zhenw@chromium.org changed reviewers:
+ brettw@chromium.org, sievers@chromium.org, stevenjb@chromium.org

[Zhen Wang, 2015-12-10 18:30:46]

Chatted with Oystein offline and he's ok with removing IssueClockSyncMarker. Now
sending to other owners.

+stevenjb for //chromeos/dbus/
+sievers for //content/public
+brettw for //tools/gn/bootstrap/bootstrap.py

[no sievers, 2015-12-10 19:35:20]

https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
content/public/browser/tracing_controller.h:47: GetAdditionalTracingAgentTrace()
const;
can you document what the two strings in the returned map are.
And also document somewhere what an agent is and what this method does? Maybe
then the interesting method name will make some sense (i.e. why 'additional',
why 'tracing agent' and why 'trace') :)

[stevenjb, 2015-12-10 20:46:40]

https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
chromeos/dbus/debug_daemon_client.cc:224:
DCHECK(stop_agent_tracing_task_runner_);
Since stop_agent_tracing_task_runner_ is required, shouldn't it be passed to the
constructor? I don't actually see where this is getting called.

[Zhen Wang, 2015-12-10 21:09:27]

https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
File chromeos/dbus/debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
chromeos/dbus/debug_daemon_client.cc:224:
DCHECK(stop_agent_tracing_task_runner_);
On 2015/12/10 20:46:40, stevenjb wrote:
> Since stop_agent_tracing_task_runner_ is required, shouldn't it be passed to
the
> constructor? I don't actually see where this is getting called.

It is passed to pipe_reader_ when creating PipeReaderForString below.

SetStopAgentTracingTaskRunner() is called right before StopAgentTracing() is
called in tracing_controller_impl.cc. And the task runner is created by calling
BrowserThread::GetBlockingPool(), which is in //content/. Is there a way to
create it in the DebugDaemonClientImpl constructor?

https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
content/public/browser/tracing_controller.h:47: GetAdditionalTracingAgentTrace()
const;
On 2015/12/10 19:35:20, sievers wrote:
> can you document what the two strings in the returned map are.
> And also document somewhere what an agent is and what this method does? Maybe
> then the interesting method name will make some sense (i.e. why 'additional',
> why 'tracing agent' and why 'trace') :)

Done. This function should corresponds to AddAgentTrace(). Since it is only
called by its child classes, I make it protected now.

[no sievers, 2015-12-11 18:13:52]

On 2015/12/10 21:09:27, Zhen Wang wrote:
>
https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
> File chromeos/dbus/debug_daemon_client.cc (right):
> 
>
https://codereview.chromium.org/1468173003/diff/220001/chromeos/dbus/debug_da...
> chromeos/dbus/debug_daemon_client.cc:224:
> DCHECK(stop_agent_tracing_task_runner_);
> On 2015/12/10 20:46:40, stevenjb wrote:
> > Since stop_agent_tracing_task_runner_ is required, shouldn't it be passed to
> the
> > constructor? I don't actually see where this is getting called.
> 
> It is passed to pipe_reader_ when creating PipeReaderForString below.
> 
> SetStopAgentTracingTaskRunner() is called right before StopAgentTracing() is
> called in tracing_controller_impl.cc. And the task runner is created by
calling
> BrowserThread::GetBlockingPool(), which is in //content/. Is there a way to
> create it in the DebugDaemonClientImpl constructor?
> 
>
https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
> File content/public/browser/tracing_controller.h (right):
> 
>
https://codereview.chromium.org/1468173003/diff/220001/content/public/browser...
> content/public/browser/tracing_controller.h:47:
GetAdditionalTracingAgentTrace()
> const;
> On 2015/12/10 19:35:20, sievers wrote:
> > can you document what the two strings in the returned map are.
> > And also document somewhere what an agent is and what this method does?
Maybe
> > then the interesting method name will make some sense (i.e. why
'additional',
> > why 'tracing agent' and why 'trace') :)
> 
> Done. This function should corresponds to AddAgentTrace(). Since it is only
> called by its child classes, I make it protected now.

content/public lgtm

[Zhen Wang, 2015-12-14 17:21:33]

ping stevenjb and brettw :)

[stevenjb, 2015-12-14 18:22:40]

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:24: // TracingController.
s/the entity/an entity/
s/And all//All/
s/are and should only/must only/
s/the TracingController/TracingController/ (or 'the TracingController instance')

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:25: //
No blank line in single comment block

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:40: virtual ~TracingAgent() {}
Since this is not a pure interface class (last two methods are implemented),
don't inline the destructor.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:42: // Get the name of the tracing agent. Each
tracing agent's name should be
Gets

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:46: // Get the trace event label of this
tracing agent. The label will be used to
Gets

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:49: //
Avoid blank lines in comment blocks unless they are truly length such as with 
RecordClockSyncMarker below.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:50: // Multiple tracing agents could have the
same label. But the tracing agents
s/But the/The/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:53: // "systemTraceEvents" as the label. And
those two agents never run at the
s/And those/Those/
(Last sentence isn't actually necessary, it should be obvious to the reader).

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:57: // Start tracing on the tracing agent with
the trace configuration.
s/Start/Starts/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:60: // Stop tracing on the tracing agent. The
trace data will be passed back to
s/Stop/Stops/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:64: // Check if the tracing agent supports
explicit clock synchronization.
s/Check/Checks/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:67: // Record a clock sync marker issued by
another tracing agent. This is only
s/Record/Records/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:77: // The 2nd thing measures that
communication latency.
s/thing/task/

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:79: // Having a reliable timing measurement for
the 2nd thing requires synchronous
task

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:82: // needs to dedicate the 2nd thing to the
receiver to take time measurements
task

https://codereview.chromium.org/1468173003/diff/260001/chromeos/dbus/fake_deb...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/260001/chromeos/dbus/fake_deb...
chromeos/dbus/fake_debug_daemon_client.cc:70: scoped_refptr<base::TaskRunner>
task_runner) {}
optional: For better testing we might want to store task_runner and use it to
invoke |callback| in StopAgentTracing() to make that asynchronous.

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:233: }
Can this be:
DebugDaemonClient debug_daemon =
chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
if (debug_daemon) {
  debug_daemon->SetStopAgentTracingTaskRunner(BrowserThread::GetBlockingPool());
  debug_daemon->StartAgentTracing(trace_config)
}

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:317: if
(it->GetTracingAgentName() == kCrOSTracingAgentName) {
This is awkward. See note above.

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:916: it !=
trace_message_filters_.end(); ++it) {
for (auto it : trace_message_filters_)

[Zhen Wang, 2015-12-14 21:03:57]

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
File base/trace_event/tracing_agent.h (right):

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:24: // TracingController.
On 2015/12/14 18:22:40, stevenjb wrote:
> s/the entity/an entity/
> s/And all//All/
> s/are and should only/must only/
> s/the TracingController/TracingController/ (or 'the TracingController
instance')
> 

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:25: //
On 2015/12/14 18:22:39, stevenjb wrote:
> No blank line in single comment block

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:40: virtual ~TracingAgent() {}
On 2015/12/14 18:22:39, stevenjb wrote:
> Since this is not a pure interface class (last two methods are implemented),
> don't inline the destructor.

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:42: // Get the name of the tracing agent. Each
tracing agent's name should be
On 2015/12/14 18:22:39, stevenjb wrote:
> Gets

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:46: // Get the trace event label of this
tracing agent. The label will be used to
On 2015/12/14 18:22:40, stevenjb wrote:
> Gets

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:49: //
On 2015/12/14 18:22:40, stevenjb wrote:
> Avoid blank lines in comment blocks unless they are truly length such as with 
> RecordClockSyncMarker below.

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:50: // Multiple tracing agents could have the
same label. But the tracing agents
On 2015/12/14 18:22:39, stevenjb wrote:
> s/But the/The/

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:53: // "systemTraceEvents" as the label. And
those two agents never run at the
On 2015/12/14 18:22:39, stevenjb wrote:
> s/And those/Those/
> (Last sentence isn't actually necessary, it should be obvious to the reader).
> 

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:57: // Start tracing on the tracing agent with
the trace configuration.
On 2015/12/14 18:22:40, stevenjb wrote:
> s/Start/Starts/

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:60: // Stop tracing on the tracing agent. The
trace data will be passed back to
On 2015/12/14 18:22:40, stevenjb wrote:
> s/Stop/Stops/

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:64: // Check if the tracing agent supports
explicit clock synchronization.
On 2015/12/14 18:22:40, stevenjb wrote:
> s/Check/Checks/

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:67: // Record a clock sync marker issued by
another tracing agent. This is only
On 2015/12/14 18:22:40, stevenjb wrote:
> s/Record/Records/

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:77: // The 2nd thing measures that
communication latency.
On 2015/12/14 18:22:40, stevenjb wrote:
> s/thing/task/
> 
> 
> 

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:79: // Having a reliable timing measurement for
the 2nd thing requires synchronous
On 2015/12/14 18:22:40, stevenjb wrote:
> task

Done.

https://codereview.chromium.org/1468173003/diff/260001/base/trace_event/traci...
base/trace_event/tracing_agent.h:82: // needs to dedicate the 2nd thing to the
receiver to take time measurements
On 2015/12/14 18:22:39, stevenjb wrote:
> task

Done.

https://codereview.chromium.org/1468173003/diff/260001/chromeos/dbus/fake_deb...
File chromeos/dbus/fake_debug_daemon_client.cc (right):

https://codereview.chromium.org/1468173003/diff/260001/chromeos/dbus/fake_deb...
chromeos/dbus/fake_debug_daemon_client.cc:70: scoped_refptr<base::TaskRunner>
task_runner) {}
On 2015/12/14 18:22:40, stevenjb wrote:
> optional: For better testing we might want to store task_runner and use it to
> invoke |callback| in StopAgentTracing() to make that asynchronous.

Acknowledged.

Maybe I should leave it to CrOS folks to improve the testing?

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:233: }
On 2015/12/14 18:22:40, stevenjb wrote:
> Can this be:
> DebugDaemonClient debug_daemon =
> chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
> if (debug_daemon) {
>  
debug_daemon->SetStopAgentTracingTaskRunner(BrowserThread::GetBlockingPool());
>   debug_daemon->StartAgentTracing(trace_config)
> }

Done.

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:317: if
(it->GetTracingAgentName() == kCrOSTracingAgentName) {
On 2015/12/14 18:22:40, stevenjb wrote:
> This is awkward. See note above.

Done.

https://codereview.chromium.org/1468173003/diff/260001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:916: it !=
trace_message_filters_.end(); ++it) {
On 2015/12/14 18:22:40, stevenjb wrote:
> for (auto it : trace_message_filters_)

Done.

[stevenjb, 2015-12-15 17:06:19]

lgtm

[brettw, 2015-12-15 17:51:50]

LGTM for gn/bootstrap

[Zhen Wang, 2015-12-15 17:53:27]

The CQ bit was checked by zhenw@chromium.org

[Zhen Wang, 2015-12-15 17:53:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
simonhatch@chromium.org, oysteine@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1468173003/#ps280001
(title: "review fix")

[commit-bot: I haz the power, 2015-12-15 17:54:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1468173003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1468173003/280001

[commit-bot: I haz the power, 2015-12-15 18:07:34]

Description was changed from

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

to

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

[commit-bot: I haz the power, 2015-12-15 18:07:35]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-12-15 18:09:17]

Description was changed from

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390
==========

to

==========
[Tracing Clock Sync] Add TracingAgent interface in Chrome

This CL adds TracingAgent interface in Chrome and hooks up
corresponding subclasses.

Design doc: https://goo.gl/8Vy6qX

BUG=542390

Committed: https://crrev.com/b0b2a7f0fd7f6ec4a43f735e643a1f2f90351dc8
Cr-Commit-Position: refs/heads/master@{#365284}
==========

[commit-bot: I haz the power, 2015-12-15 18:09:18]

Patchset 15 (id:??) landed as
https://crrev.com/b0b2a7f0fd7f6ec4a43f735e643a1f2f90351dc8
Cr-Commit-Position: refs/heads/master@{#365284}