Implement sampling profiler (trace-viewer side change)

Implement sampling profiler (trace-viewer side change)

Chromium side change is here: https://codereview.chromium.org/109933006/

Screenshot of the profiler: http://haraken.info/null/sampling_tracing.png

Design document: https://docs.google.com/a/google.com/document/d/1j39sbA9ECTwFFlxbhSD1EvJnGfYeS5EUJ05yxuku6VY/edit

[haraken, 2013-12-11 04:57:17]

This is the first version of the sampling profiler.

Would you take a quick look at the CL? If it looks good, I'll add test cases.

FYI, chromium side change is here: https://codereview.chromium.org/109933006/

[dsinclair, 2013-12-11 16:04:40]

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:171:
buttons.querySelector('#record-button').disabled = false;
If you moved this code into the promise error callback it would be the same as
the error callback above, could we move those to a common routine?

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:192: //console.log(data);
remove.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
File src/about_tracing/tracing_requestor.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:41: categoryFilter: "-*",
So, everything is disabled?

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:54: function beginMonitoringResolved() {
Why make real functions and not use anonymous ones?

[haraken, 2013-12-12 00:51:57]

Thanks for comments! Let me add tests and ping you.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
File src/about_tracing/tracing_requestor.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:41: categoryFilter: "-*",
On 2013/12/11 16:04:40, dsinclair wrote:
> So, everything is disabled?

I'm not sure if this is a good approach, but this will realize the behavior that
we desire.

See a discussion with nduca@:
https://code.google.com/p/trace-viewer/issues/detail?id=510

[nduca, 2013-12-12 06:56:50]

Wow this is so cool!

I think there are a few patches here that you could land really easily and I'm
happy to rubberstamp (etc). I am afraid I do have a few more annoying thoughts
to share on the profiling_view etc changes though, noted below. But, this is a
huge step forward, I have hope we can land something relatively soon assuming
you can still manage to find time!

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.html (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.html:27: <button id="monitor-button">Enable
Monitoring</button>
seems like we should make this be a checkbox?

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.html:28: <button id="capture-button">Capture
Monitoring Snapshot</button>
you also need a configure-monitoring-button here

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:84: this.isMonitoring_ = false;
This is a bit racey when two about:tracing tabs are open. Even racier when there
are two about:tracing windows open (in the tab case we could listen to window
visibility hint maybe to tell when to update the state?). I made some notes on
hardening for this general race condition below.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:137: 
I think some architecture changes are required in this patch. THe problem with
the approach here is that the monitoring category has a lot of race conditions
in it and issues that happen if there are more than one about:tracing windows
open and people are interacting with them. This happens surprisingly often so we
must design a system that behaves well when there are race conditions.

I propose that the fix for this is that tracing_controller should have an actual
state about what monitoring categories are on,
TracingController::SetMonitoringCategoryFilter(),
Tracingcontroller::GetMonitoringCategoryFilter(). This only applies to **for
monitoring** mode. It could default to -* if you wanted. If
SetMonitoringCategoryFilter is called while monitoring is on, then the
controller should  stop monitoring, configure, and start monitoring again.

The beginMonitoring call from the UI shouldn't include the filter. When you
begin monitoring, it just uses the previously-configured monitoring category.
That way if someone configures in one tab and then turns monitoring on elsewhere
it has a chance of behaving correctly.

Clicking the configure button should query the tracing_controller for the
current monitoring filter, and then show a RecordingSelectionDialog with the
response. When the dialog is hidden, you should send a setMonitoringFilter call
to the tracing_ui.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:141: if (this.isMonitoring_)
We might also want to make the tracing_ui.cc to listen for monitoring enabld
state changes to the tracing controller, and when it does change, it should call
ExecuteJavascript("document.dispatchEvent(new Event('monitoringStateChanged'))"
and then have profilnig_view.js listen for that event and update the monitoring
checkbox accorrdingly. Again, this is part of making this part of the system
robust.

If you have idaes on how to do this incrementally I'd be open to it. :) I'd like
each incrmeental step to be free of race conditions and not undo the previous
api entries added, if possible.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
File src/about_tracing/tracing_requestor.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:41: categoryFilter: "-*",
I left some notes in the previous section. I'm okay starting with -* as a
default but the state must be c++ side, and we should probalby implement the
configure button so that we get the right features and race conditions resolved
in this version.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/analysis/analy...
File src/tracing/analysis/analysis_results.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/analysis/analy...
src/tracing/analysis/analysis_results.js:6: 
If you want to pull out the analysis changes and track changes to a separate
patch i think we can land those pretty easily. they look really nice.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/timeline_view.js
File src/tracing/timeline_view.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/timeline_view....
src/tracing/timeline_view.js:67: this.onRequestSelectionChange_.bind(this));
i think if you took this plus the track changes, they'd make a good patch "allow
tracks to change selection too"

https://codereview.appspot.com/39310043/diff/40001/src/tracing/trace_model/pr...
File src/tracing/trace_model/process.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/trace_model/pr...
src/tracing/trace_model/process.js:82: }
want to do this as a standalone patch, tbr nduca? Bonus points for test. I made
your chromium.org a committer on trace-viewer so you're good to go.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
File src/tracing/tracks/heading_track.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
src/tracing/tracks/heading_track.js:48: set selectionGenerator(generator) {
a docstring would probably be helpful to explain what this does.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
src/tracing/tracks/heading_track.js:49: if (this.headingDiv_.firstChild ===
this.textContent_) {
i think the traditional way we write this in trace viewer is to store the two
properties that affect the dom --- heading and selectionGenerator, then make
each of the setters call updateContents_().

Constructor:
  this.heading_ = '';
  this.selectionGenerator_ = undefined;
  this.headingDiv_ = as before
  this.updateContents_();

setters set the right thing then updateContents

updateContents_' job is to look at headingDIv_ and make sure its either a
AnalysisLink or a text element and then set the text content.

This design pattern ensures that all the combinations work --- setting
selectionGenerator(undefined) still works, etc, but then whats happening with
changing the dom is easier to read. It took me an embarassingly long time to
figure out what was going on here.

https://codereview.appspot.com/39310043/diff/40001/third_party/Promises/polyf...
File third_party/Promises/polyfill/src/Promise.js (right):

https://codereview.appspot.com/39310043/diff/40001/third_party/Promises/polyf...
third_party/Promises/polyfill/src/Promise.js:251: console.log(e);
feel free to tbr=nduca this change :)

[haraken, 2013-12-12 07:29:57]

Thanks Nat for detailed review! I'll split the CL into pieces and start landing
them from easier parts.

[haraken, 2013-12-25 13:10:18]

dsinclair, wang, nduca: Let me ask one question.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:141: if (this.isMonitoring_)
On 2013/12/12 06:56:51, nduca wrote:
> We might also want to make the tracing_ui.cc to listen for monitoring enabld
> state changes to the tracing controller, and when it does change, it should
call
> ExecuteJavascript("document.dispatchEvent(new
Event('monitoringStateChanged'))"
> and then have profilnig_view.js listen for that event and update the
monitoring
> checkbox accorrdingly. Again, this is part of making this part of the system
> robust.

I'm thinking about how to address this issue.

How can we invoke ExecuteJavascript("document.dispatchEvent(new
Event('monitoringStateChanged'))" from tracing_controller_impl.cc? Remember that
there are multiple about:tracings running in Chrome. This means that we need to
dispatch ExecuteJavaScript for all documents of the about:tracings. We can do it
by having tracing_controller_impl.cc keep track of all WebUIImpls of the
about:tracings, but it would be ugly.

Another idea is adding "json/observe_monitoring_state". Specifically, we can
implement the following interaction:

(1) profiling_view.js sends an XHR with "json/observe_monitoring_state". The XHR
holds a callback X in profiling_view.js.
(2) tracing_ui.cc accepts the XHR, and registers the callback X to
tracing_controller_impl.cc.
(3) Every time tracing_controller_impl.cc changes the monitoring mode, it calls
back X. Thus profiling_view.js can know that the monitoring mode has changed.

However, this approach doesn't work, because one XHR can receive only one
response. We need a mechanism to register the callback X once and let the
callback X be called back multiple times from tracing_controller_impl.cc.

Do you have any idea?

[haraken, 2014-01-04 09:21:14]

On 2013/12/25 13:10:18, haraken wrote:
> dsinclair, wang, nduca: Let me ask one question.
> 
>
https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
> File src/about_tracing/profiling_view.js (right):
> 
>
https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
> src/about_tracing/profiling_view.js:141: if (this.isMonitoring_)
> On 2013/12/12 06:56:51, nduca wrote:
> > We might also want to make the tracing_ui.cc to listen for monitoring enabld
> > state changes to the tracing controller, and when it does change, it should
> call
> > ExecuteJavascript("document.dispatchEvent(new
> Event('monitoringStateChanged'))"
> > and then have profilnig_view.js listen for that event and update the
> monitoring
> > checkbox accorrdingly. Again, this is part of making this part of the system
> > robust.
> 
> I'm thinking about how to address this issue.
> 
> How can we invoke ExecuteJavascript("document.dispatchEvent(new
> Event('monitoringStateChanged'))" from tracing_controller_impl.cc? Remember
that
> there are multiple about:tracings running in Chrome. This means that we need
to
> dispatch ExecuteJavaScript for all documents of the about:tracings. We can do
it
> by having tracing_controller_impl.cc keep track of all WebUIImpls of the
> about:tracings, but it would be ugly.
> 
> Another idea is adding "json/observe_monitoring_state". Specifically, we can
> implement the following interaction:
> 
> (1) profiling_view.js sends an XHR with "json/observe_monitoring_state". The
XHR
> holds a callback X in profiling_view.js.
> (2) tracing_ui.cc accepts the XHR, and registers the callback X to
> tracing_controller_impl.cc.
> (3) Every time tracing_controller_impl.cc changes the monitoring mode, it
calls
> back X. Thus profiling_view.js can know that the monitoring mode has changed.
> 
> However, this approach doesn't work, because one XHR can receive only one
> response. We need a mechanism to register the callback X once and let the
> callback X be called back multiple times from tracing_controller_impl.cc.
> 
> Do you have any idea?

Ping on this?

[haraken, 2014-01-06 17:03:47]

On 2014/01/04 09:21:14, haraken wrote:
> On 2013/12/25 13:10:18, haraken wrote:
> > dsinclair, wang, nduca: Let me ask one question.
> > 
> >
>
https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
> > File src/about_tracing/profiling_view.js (right):
> > 
> >
>
https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
> > src/about_tracing/profiling_view.js:141: if (this.isMonitoring_)
> > On 2013/12/12 06:56:51, nduca wrote:
> > > We might also want to make the tracing_ui.cc to listen for monitoring
enabld
> > > state changes to the tracing controller, and when it does change, it
should
> > call
> > > ExecuteJavascript("document.dispatchEvent(new
> > Event('monitoringStateChanged'))"
> > > and then have profilnig_view.js listen for that event and update the
> > monitoring
> > > checkbox accorrdingly. Again, this is part of making this part of the
system
> > > robust.
> > 
> > I'm thinking about how to address this issue.
> > 
> > How can we invoke ExecuteJavascript("document.dispatchEvent(new
> > Event('monitoringStateChanged'))" from tracing_controller_impl.cc? Remember
> that
> > there are multiple about:tracings running in Chrome. This means that we need
> to
> > dispatch ExecuteJavaScript for all documents of the about:tracings. We can
do
> it
> > by having tracing_controller_impl.cc keep track of all WebUIImpls of the
> > about:tracings, but it would be ugly.
> > 
> > Another idea is adding "json/observe_monitoring_state". Specifically, we can
> > implement the following interaction:
> > 
> > (1) profiling_view.js sends an XHR with "json/observe_monitoring_state". The
> XHR
> > holds a callback X in profiling_view.js.
> > (2) tracing_ui.cc accepts the XHR, and registers the callback X to
> > tracing_controller_impl.cc.
> > (3) Every time tracing_controller_impl.cc changes the monitoring mode, it
> calls
> > back X. Thus profiling_view.js can know that the monitoring mode has
changed.
> > 
> > However, this approach doesn't work, because one XHR can receive only one
> > response. We need a mechanism to register the callback X once and let the
> > callback X be called back multiple times from tracing_controller_impl.cc.
> > 
> > Do you have any idea?

nduca: Would you give me advice about how to observe
TracingControllerImpl::is_monitoring from trace-viewer JavaScripts?

[nduca, 2014-01-07 21:36:32]

I think there is a TracingUI instance for every chrome://tracing tab. If you
make that register with the tracing controller in constructor, and unregister in
destructor, would that work? TracingUI has a way to ExecuteJavascript, I'm
pretty sure.

[haraken, 2014-01-08 13:09:34]

Thanks Nat for advice!

I implemented a mechanism to let TracingController invoke JavaScript via
TracingUI in https://codereview.chromium.org/127703002/. Also I updated this CL
using the mechanism so that there is no race issues in isMonitoring_.

PTAL.

If this CL looks almost OK,

- I'll add test cases.
- I'll split the CL into the analysis part and the track part.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.html (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.html:27: <button id="monitor-button">Enable
Monitoring</button>
On 2013/12/12 06:56:51, nduca wrote:
> seems like we should make this be a checkbox?

Done.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.html:28: <button id="capture-button">Capture
Monitoring Snapshot</button>
On 2013/12/12 06:56:51, nduca wrote:
> you also need a configure-monitoring-button here

Yes, but let me do that in a separate CL. This CL assumes the monitoring mode
starts the sampling thread with a category filter "*".

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
File src/about_tracing/profiling_view.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:84: this.isMonitoring_ = false;
On 2013/12/12 06:56:51, nduca wrote:
> This is a bit racey when two about:tracing tabs are open. Even racier when
there
> are two about:tracing windows open (in the tab case we could listen to window
> visibility hint maybe to tell when to update the state?). I made some notes on
> hardening for this general race condition below.

Done.

https://codereview.chromium.org/127703002/ implements a mechanism to let
TracingController invoke JavaScript via TracingUI. This CL depends on the
mechanism and updates is_monitoring_ consistently.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:137: 
On 2013/12/12 06:56:51, nduca wrote:
> I think some architecture changes are required in this patch. THe problem with
> the approach here is that the monitoring category has a lot of race conditions
> in it and issues that happen if there are more than one about:tracing windows
> open and people are interacting with them. This happens surprisingly often so
we
> must design a system that behaves well when there are race conditions.
> 
> I propose that the fix for this is that tracing_controller should have an
actual
> state about what monitoring categories are on,
> TracingController::SetMonitoringCategoryFilter(),
> Tracingcontroller::GetMonitoringCategoryFilter(). This only applies to **for
> monitoring** mode. It could default to -* if you wanted. If
> SetMonitoringCategoryFilter is called while monitoring is on, then the
> controller should  stop monitoring, configure, and start monitoring again.
> 
> The beginMonitoring call from the UI shouldn't include the filter. When you
> begin monitoring, it just uses the previously-configured monitoring category.
> That way if someone configures in one tab and then turns monitoring on
elsewhere
> it has a chance of behaving correctly.
> 
> Clicking the configure button should query the tracing_controller for the
> current monitoring filter, and then show a RecordingSelectionDialog with the
> response. When the dialog is hidden, you should send a setMonitoringFilter
call
> to the tracing_ui.

Agreed. However, let me fix this issue when implementing the "configure" button
for the monitoring mode.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:141: if (this.isMonitoring_)
On 2013/12/12 06:56:51, nduca wrote:
> We might also want to make the tracing_ui.cc to listen for monitoring enabld
> state changes to the tracing controller, and when it does change, it should
call
> ExecuteJavascript("document.dispatchEvent(new
Event('monitoringStateChanged'))"
> and then have profilnig_view.js listen for that event and update the
monitoring
> checkbox accorrdingly. Again, this is part of making this part of the system
> robust.
> 
> If you have idaes on how to do this incrementally I'd be open to it. :) I'd
like
> each incrmeental step to be free of race conditions and not undo the previous
> api entries added, if possible.

Done.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:171:
buttons.querySelector('#record-button').disabled = false;
On 2013/12/11 16:04:40, dsinclair wrote:
> If you moved this code into the promise error callback it would be the same as
> the error callback above, could we move those to a common routine?

Done.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/profilin...
src/about_tracing/profiling_view.js:192: //console.log(data);
On 2013/12/11 16:04:40, dsinclair wrote:
> remove.

Done.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
File src/about_tracing/tracing_requestor.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:41: categoryFilter: "-*",
On 2013/12/12 06:56:51, nduca wrote:
> I left some notes in the previous section. I'm okay starting with -* as a
> default but the state must be c++ side, and we should probalby implement the
> configure button so that we get the right features and race conditions
resolved
> in this version.

This issue was already addressed. Now we can use "*" or whatever else.

https://codereview.appspot.com/39310043/diff/40001/src/about_tracing/tracing_...
src/about_tracing/tracing_requestor.js:54: function beginMonitoringResolved() {
On 2013/12/11 16:04:40, dsinclair wrote:
> Why make real functions and not use anonymous ones?

Done.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/analysis/analy...
File src/tracing/analysis/analysis_results.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/analysis/analy...
src/tracing/analysis/analysis_results.js:6: 
On 2013/12/12 06:56:51, nduca wrote:
> If you want to pull out the analysis changes and track changes to a separate
> patch i think we can land those pretty easily. they look really nice.

Sure. If the overall CL looks OK, I'll split the CL and add test cases.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/trace_model/pr...
File src/tracing/trace_model/process.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/trace_model/pr...
src/tracing/trace_model/process.js:82: }
On 2013/12/12 06:56:51, nduca wrote:
> want to do this as a standalone patch, tbr nduca? Bonus points for test. I
made
> your http://chromium.org a committer on trace-viewer so you're good to go.

This was already addressed in another CL.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
File src/tracing/tracks/heading_track.js (right):

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
src/tracing/tracks/heading_track.js:48: set selectionGenerator(generator) {
On 2013/12/12 06:56:51, nduca wrote:
> a docstring would probably be helpful to explain what this does.

Done.

https://codereview.appspot.com/39310043/diff/40001/src/tracing/tracks/heading...
src/tracing/tracks/heading_track.js:49: if (this.headingDiv_.firstChild ===
this.textContent_) {
On 2013/12/12 06:56:51, nduca wrote:
> i think the traditional way we write this in trace viewer is to store the two
> properties that affect the dom --- heading and selectionGenerator, then make
> each of the setters call updateContents_().
> 
> Constructor:
>   this.heading_ = '';
>   this.selectionGenerator_ = undefined;
>   this.headingDiv_ = as before
>   this.updateContents_();
> 
> setters set the right thing then updateContents
> 
> updateContents_' job is to look at headingDIv_ and make sure its either a
> AnalysisLink or a text element and then set the text content.
> 
> This design pattern ensures that all the combinations work --- setting
> selectionGenerator(undefined) still works, etc, but then whats happening with
> changing the dom is easier to read. It took me an embarassingly long time to
> figure out what was going on here.

Done.

https://codereview.appspot.com/39310043/diff/40001/third_party/Promises/polyf...
File third_party/Promises/polyfill/src/Promise.js (right):

https://codereview.appspot.com/39310043/diff/40001/third_party/Promises/polyf...
third_party/Promises/polyfill/src/Promise.js:251: console.log(e);
On 2013/12/12 06:56:51, nduca wrote:
> feel free to tbr=nduca this change :)

This was already addressed in another CL.

[nduca, 2014-01-09 01:11:52]

lgtm overall with the comments you made

https://codereview.appspot.com/39310043/diff/120001/src/about_tracing/tracing...
File src/about_tracing/tracing_requestor.js (left):

https://codereview.appspot.com/39310043/diff/120001/src/about_tracing/tracing...
src/about_tracing/tracing_requestor.js:6: 
lets call this file tracing_ui_client.js or something since it talks to
tracing_ui?

[haraken, 2014-01-09 07:32:37]

Thanks Nat for quick review!

I added test cases and split the CL into two pieces:

about_tracing part: https://codereview.appspot.com/49660043/
tracing part: https://codereview.appspot.com/49500046/

>
https://codereview.appspot.com/39310043/diff/120001/src/about_tracing/tracing...
> File src/about_tracing/tracing_requestor.js (left):
> 
>
https://codereview.appspot.com/39310043/diff/120001/src/about_tracing/tracing...
> src/about_tracing/tracing_requestor.js:6: 
> lets call this file tracing_ui_client.js or something since it talks to
> tracing_ui?

Done.

[nduca, 2014-01-09 17:28:05]

is this one closed?

[haraken, 2014-01-10 01:03:57]

On 2014/01/09 17:28:05, nduca wrote:
> is this one closed?

closed.