Tidy up the debugger.

Tidy up the debugger.

Remove dependence on WindowManager.
Remove unused profiling option.
Fix crash in trace collector when it receives multiple connections.

R=abarth@google.com, ppi@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/e4f203d8a4dd9233e54de390a73f2e7c9094e687

[viettrungluu, 2015-12-17 22:43:04]

viettrungluu@chromium.org changed reviewers:
+ ppi@chromium.org

[viettrungluu, 2015-12-17 22:43:05]

+ppi

Does the stuff in mojo/devtools/common/mojo_{debug,run} need to be updated (or
deleted) too?

(Is mojo:debugger really totally unused?)

[ppi, 2015-12-18 09:36:12]

not lgtm

This service is used by mojo_debug to drive interactive tracing, see
https://github.com/domokit/mojo/blob/master/mojo/devtools/common/docs/mojo_de...
. Just tried it on ToT and seems to be working as expected.

Please let me know if I am missing something, but it seems that this should not
be affected by the switch to Mozart.

[jeffbrown, 2015-12-18 17:52:54]

On 2015/12/18 09:36:12, ppi wrote:
> not lgtm
> 
> This service is used by mojo_debug to drive interactive tracing, see
>
https://github.com/domokit/mojo/blob/master/mojo/devtools/common/docs/mojo_de...
> . Just tried it on ToT and seems to be working as expected.
> 
> Please let me know if I am missing something, but it seems that this should
not
> be affected by the switch to Mozart.

Are the GUI parts of it being used?  The sky folks seemed to think this was
superceded by other tooling, but perhaps the name is misleading.

[ppi, 2015-12-18 18:03:34]

Oh, I forgot we still have these wm bits, thanks for clarification.

Deleting the Load / Reload chunks sg. please remove also the corresponding
wm_load (and entire wm command group) from
https://github.com/domokit/mojo/blob/master/mojo/devtools/common/mojo_debug .

[jeffbrown, 2015-12-18 22:42:35]

Description was changed from

==========
Remove defunct skydb debugger.

BUG=

Review URL: https://codereview.chromium.org/1533563004 .

R=abarth, jamesr, viettrungluu
==========

to

==========
Tidy up the debugger.

Remove dependence on WindowManager.
Remove unused profiling option.
Fix crash in trace collector when it receives multiple connections.
==========

[jeffbrown, 2015-12-18 22:43:38]

Cleaned things up a bit.  I also removed the unused profiling code but can put
it back if you like.  (Nothing in the Mojo tree calls it, ever.)

[ppi, 2015-12-20 15:36:21]

Thanks! The debugger part lg, but ptal at the comment regarding tracing_app
below.

https://codereview.chromium.org/1532893003/diff/20001/services/debugger/debug...
File services/debugger/debugger.cc (right):

https://codereview.chromium.org/1532893003/diff/20001/services/debugger/debug...
services/debugger/debugger.cc:101: "Sky Debugger running on port %d\n"
can you also s/Sky // ?

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
File services/tracing/tracing_app.h (right):

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
services/tracing/tracing_app.h:48: mojo::BindingSet<TraceCollector>
collector_bindings_;
Collector instances are "stateful", it is incorrect to bind independent requests
to the same instance.

[jeffbrown, 2015-12-20 20:52:59]

https://codereview.chromium.org/1532893003/diff/20001/services/debugger/debug...
File services/debugger/debugger.cc (right):

https://codereview.chromium.org/1532893003/diff/20001/services/debugger/debug...
services/debugger/debugger.cc:101: "Sky Debugger running on port %d\n"
On 2015/12/20 15:36:21, ppi wrote:
> can you also s/Sky // ?

Sure.

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
File services/tracing/tracing_app.h (right):

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
services/tracing/tracing_app.h:48: mojo::BindingSet<TraceCollector>
collector_bindings_;
On 2015/12/20 15:36:21, ppi wrote:
> Collector instances are "stateful", it is incorrect to bind independent
requests
> to the same instance.

Ahhh, then this whole class is somewhat problematic.  The service is trying to
behave like a singleton yet it crashes when multiple connections are made to it.

Specifically, if you start tracing when launching an app you are unable to
restart tracing later using the mojo_debug tool.

[ppi, 2015-12-21 09:39:50]

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
File services/tracing/tracing_app.h (right):

https://codereview.chromium.org/1532893003/diff/20001/services/tracing/tracin...
services/tracing/tracing_app.h:48: mojo::BindingSet<TraceCollector>
collector_bindings_;
On 2015/12/20 20:52:58, jeffbrown wrote:
> On 2015/12/20 15:36:21, ppi wrote:
> > Collector instances are "stateful", it is incorrect to bind independent
> requests
> > to the same instance.
> 
> Ahhh, then this whole class is somewhat problematic.  The service is trying to
> behave like a singleton yet it crashes when multiple connections are made to
it.
> 
> Specifically, if you start tracing when launching an app you are unable to
> restart tracing later using the mojo_debug tool.

Yes, and the problems go deeper, down to C++ base:: implementation of tracing,
which can't do two overlapping tracing runs.

I think we should print a better error message saying that tracing can support
only one trace collection at a time when subsequent connection is made, and
close the passed message pipe.

[ppi, 2015-12-31 10:19:49]

lgtm, thanks!

[abarth, 2016-01-10 01:01:27]

LGTM

[jeffbrown, 2016-01-26 08:44:46]

rebase

[jeffbrown, 2016-01-26 23:48:23]

Description was changed from

==========
Tidy up the debugger.

Remove dependence on WindowManager.
Remove unused profiling option.
Fix crash in trace collector when it receives multiple connections.
==========

to

==========
Tidy up the debugger.

Remove dependence on WindowManager.
Remove unused profiling option.
Fix crash in trace collector when it receives multiple connections.

R=abarth@google.com, ppi@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/e4f203d8a4dd9233e54de390a73...
==========

[jeffbrown, 2016-01-26 23:48:25]

Committed patchset #4 (id:60001) manually as
e4f203d8a4dd9233e54de390a73f2e7c9094e687 (presubmit successful).