Add shell::Tracer object and wire up to --trace-startup on Android

Add shell::Tracer object and wire up to --trace-startup on Android

This adds an object to keep track of tracing state and wires it into
the android mojo shell main. When --trace-startup is passed in on the
command line (or via the intent extra args thingy on Android) the shell
enables tracing for itself. It stops tracing when it shuts down cleanly
or 5 seconds elapse. On desktop, this writes into a file named
"mojo_shell.trace" in the CWD. On Android, this writes to
/data/data/org.chromium.mojo.shell/cache/tmp/mojo_shell.trace.

Follow-up patches will patch in trace data from the tracing service into
this file.

R=qsr@chromium.org, viettrungluu@chromium.org

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

[jamesr, 2015-04-07 20:22:37]

jamesr@chromium.org changed reviewers:
+ qsr@chromium.org

[jamesr, 2015-04-07 20:22:38]

vtl - please review
qsr - FYI for android shell changes

[jamesr, 2015-04-07 20:23:37]

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.cc
File shell/tracer.cc (right):

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.cc#newcode80
shell/tracer.cc:80: flush_complete_event.Wait();
if the message loop is running (i.e. we're stopping tracing due to the timeout
elapsing and not shutdown) then this really should spin a RunLoop to not freeze
up the thread. will fix in a follow-up

[jamesr, 2015-04-07 20:33:57]

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.cc
File shell/tracer.cc (right):

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.cc#newcode22
shell/tracer.cc:22: printf("tracing started\n");
whoops, didn't mean to check these in. will delete. (maybe that presubmit is
useful?)

[jamesr, 2015-04-07 22:02:34]

spurious printf()s gone, PTAL

[viettrungluu, 2015-04-07 22:05:44]

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT.py#newcode1028
PRESUBMIT.py:1028: def _GetJSONParseError(input_api, filename,
eat_comments=True):
Remove the eat_comments arg?

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT_test.py
File PRESUBMIT_test.py (left):

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT_test.py#oldcode437
PRESUBMIT_test.py:437: def testNoEatComments(self):
Maybe keep this test (and rename it)?

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h
File shell/tracer.h (right):

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode16
shell/tracer.h:16: namespace shell {
For consistency, could you put this in the mojo::shell namespace until I get
around to mass-converting everything?

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode35
shell/tracer.h:35: void StopAndFlushToFile(std::string filename);
const std::string&?

Maybe also merge StopTracingAndFlushToDisk() into this, and add an early return.

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode55
shell/tracer.h:55: uint32_t blocks_written_;
size_t instead?

https://codereview.chromium.org/1067023003/diff/20001/shell/tracer.cc
File shell/tracer.cc (right):

https://codereview.chromium.org/1067023003/diff/20001/shell/tracer.cc#newcode18
shell/tracer.cc:18: Tracer::~Tracer() {
nit: blank line above?

https://codereview.chromium.org/1067023003/diff/20001/shell/tracer.cc#newcode49
shell/tracer.cc:49: if (blocks_written_) {
nit: no braces? (or braces elsewhere)

https://codereview.chromium.org/1067023003/diff/20001/shell/tracer.cc#newcode58
shell/tracer.cc:58: fwrite(kEnd, 1, strlen(kEnd), trace_file_);
nit: #include <string.h>

[jamesr, 2015-04-07 22:20:07]

thanks, PTAL

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT_test.py
File PRESUBMIT_test.py (left):

https://codereview.chromium.org/1067023003/diff/1/PRESUBMIT_test.py#oldcode437
PRESUBMIT_test.py:437: def testNoEatComments(self):
On 2015/04/07 22:05:44, viettrungluu wrote:
> Maybe keep this test (and rename it)?

At this point it's just testing the JSON parser which is input_api.json and
mocked out in this test. I don't think we really need a unit test of python's
json parser in this file

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h
File shell/tracer.h (right):

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode16
shell/tracer.h:16: namespace shell {
On 2015/04/07 22:05:44, viettrungluu wrote:
> For consistency, could you put this in the mojo::shell namespace until I get
> around to mass-converting everything?

:(

ok

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode35
shell/tracer.h:35: void StopAndFlushToFile(std::string filename);
On 2015/04/07 22:05:44, viettrungluu wrote:
> const std::string&?

done

> 
> Maybe also merge StopTracingAndFlushToDisk() into this, and add an early
return.

I'm planning to add more branches here soon (as in the next patch or so) so I
think keeping it separate is better

https://codereview.chromium.org/1067023003/diff/1/shell/tracer.h#newcode55
shell/tracer.h:55: uint32_t blocks_written_;
On 2015/04/07 22:05:44, viettrungluu wrote:
> size_t instead?

actually we just need one bit to tell if we need to write a "," before a chunk.
replaced with a bool

[viettrungluu, 2015-04-08 00:39:16]

LGTM w/printf removed.

https://codereview.chromium.org/1067023003/diff/40001/shell/tracer.cc
File shell/tracer.cc (right):

https://codereview.chromium.org/1067023003/diff/40001/shell/tracer.cc#newcode61
shell/tracer.cc:61: printf("got empty chunk, skipping. has_more_events %d\n",
has_more_events);
I don't think you really want this printf (and if you did, this else block would
need braces).

[qsr, 2015-04-08 08:55:57]

https://codereview.chromium.org/1067023003/diff/40001/shell/android/main.cc
File shell/android/main.cc (right):

https://codereview.chromium.org/1067023003/diff/40001/shell/android/main.cc#n...
shell/android/main.cc:195: base::Bind(&Tracer::StopAndFlushToFile,
base::Unretained(&tracer),
I am missing something here. tracer is a stack variable. Your method will end a
few ms after this call. How is this working?

[jamesr, 2015-04-09 23:00:04]

On 2015/04/08 08:55:57, qsr wrote:
> https://codereview.chromium.org/1067023003/diff/40001/shell/android/main.cc
> File shell/android/main.cc (right):
> 
>
https://codereview.chromium.org/1067023003/diff/40001/shell/android/main.cc#n...
> shell/android/main.cc:195: base::Bind(&Tracer::StopAndFlushToFile,
> base::Unretained(&tracer),
> I am missing something here. tracer is a stack variable. Your method will end
a
> few ms after this call. How is this working?

That's right, it needs to be in a LazyInstance since this code doesn't own the
message loop (I had this fixed in another branch but forgot to merge it back)

[jamesr, 2015-04-09 23:41:00]

PTAL

[qsr, 2015-04-10 08:08:02]

lgtm

[jamesr, 2015-04-10 18:29:52]

Committed patchset #5 (id:80001) manually as
f0f1860a52c87faa45f372e1b5b6a8948ae43249 (presubmit successful).