Add trace-to-file support for base::TestSuite

Add trace-to-file support for base::TestSuite

Passing --trace-to-file on the command line will trace a default set of
categories to a default trace file name (trace.json).
--trace-to-file=categories --trace-to-file-name=somefile.json allow for
more configuration.

BUG=none

Committed: https://crrev.com/04213de5a22ff0f95a93584cde042133c358c184
Cr-Commit-Position: refs/heads/master@{#296770}

[enne (OOO), 2014-09-05 00:54:35]

enne@chromium.org changed reviewers:
+ thakis@chromium.org

[enne (OOO), 2014-09-05 00:55:13]

I am not sure I know what I'm doing with message loops, so please give this a
skeptical eye.

[enne (OOO), 2014-09-05 23:38:57]

enne@chromium.org changed reviewers:
+ ajwong@chromium.org

[mithro-old, 2014-09-17 09:35:03]

Dana pointed out this patch and I think it is pretty awesome, I'd love to see it
land.

I'm wondering if trace-to-console is just trace-to-file with the file being
"stdout". I wouldn't block this patch on trying to figure that out however.

Tim 'mithro' Ansell

[Ian Vollick, 2014-09-17 10:50:53]

On 2014/09/17 09:35:03, mithro wrote:
> Dana pointed out this patch and I think it is pretty awesome, I'd love to see
it
> land.
> 
> I'm wondering if trace-to-console is just trace-to-file with the file being
> "stdout". I wouldn't block this patch on trying to figure that out however.
> 
> Tim 'mithro' Ansell

trace-to-console is a bit different. It petty prints the trace info (adds colors
to indicate what's happening on what thread, eg) but trace to file dumps
unmodified trace data that could be loaded by the viewer in chrome.

[awong, 2014-09-17 16:03:53]

I also like this idea. Couple of tweaks.

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file.cc
File base/debug/trace_to_file.cc (right):

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file...
base/debug/trace_to_file.cc:31: // empty filter (i.e. just --trace-to-file)
turns into default categories in
nit: empty -> Empty

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file...
base/debug/trace_to_file.cc:110: base::MessageLoop loop;
This doesn't look quite right.  
  (a) Creating RunLoops is prefer over creating nested message loops.
  (b) The RepostUntilAllDataCollected, while clever, is effectively a busy wait.

Can this pattern work?

Runloop run_loop;

base::debug::TraceLog::GetInstance()->Flush(
   Bind(&OnTraceDataCollected, Unretained(buffer), run_loop.QuitClosure());
run_loop.Run();

Then in OnTraceDataCollected, call the passed in quit closure instead of setting
all_data_collected to true.

[enne (OOO), 2014-09-24 23:01:58]

I also moved the files to base/test since it really only makes sense in test
code.

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file.cc
File base/debug/trace_to_file.cc (right):

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file...
base/debug/trace_to_file.cc:31: // empty filter (i.e. just --trace-to-file)
turns into default categories in
On 2014/09/17 at 16:03:52, awong wrote:
> nit: empty -> Empty

Done.

https://codereview.chromium.org/542893002/diff/40001/base/debug/trace_to_file...
base/debug/trace_to_file.cc:110: base::MessageLoop loop;
On 2014/09/17 at 16:03:52, awong wrote:
> This doesn't look quite right.  
>   (a) Creating RunLoops is prefer over creating nested message loops.
>   (b) The RepostUntilAllDataCollected, while clever, is effectively a busy
wait.
> 
> Can this pattern work?
> 
> Runloop run_loop;
> 
> base::debug::TraceLog::GetInstance()->Flush(
>    Bind(&OnTraceDataCollected, Unretained(buffer), run_loop.QuitClosure());
> run_loop.Run();
> 
> Then in OnTraceDataCollected, call the passed in quit closure instead of
setting all_data_collected to true.

Done.  Thanks for the pointers, that looks *way* better.  :)

[enne (OOO), 2014-09-24 23:34:53]

enne@chromium.org changed reviewers:
+ willchan@chromium.org

[enne (OOO), 2014-09-24 23:35:09]

+willchan, since ajwong is on leave, apparently.

[enne (OOO), 2014-09-24 23:36:11]

Er, apparently willchan is also gone.

thakis?

[Nico, 2014-09-24 23:47:30]

I'm away starting tomorrow :-)

Seems useful, lgtm. It depends on destructors running at test end though, which
might not always be true (it currently is)

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file.cc
File base/test/trace_to_file.cc (right):

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:43: LOG(ERROR) << "Start " << switches::kTraceToFile
<< " with CategoryFilter '"
This is not an error. Don't log unless something's wrong.

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:63: const char* str = "{\"traceEvents\": [";
const char str[]

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:68: const char* str = "]}";
ditto

[enne (OOO), 2014-09-24 23:58:57]

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file.cc
File base/test/trace_to_file.cc (right):

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:43: LOG(ERROR) << "Start " << switches::kTraceToFile
<< " with CategoryFilter '"
On 2014/09/24 23:47:30, Nico (hiding) wrote:
> This is not an error. Don't log unless something's wrong.

Done.

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:63: const char* str = "{\"traceEvents\": [";
On 2014/09/24 23:47:30, Nico (hiding) wrote:
> const char str[]

Done.

https://codereview.chromium.org/542893002/diff/120001/base/test/trace_to_file...
base/test/trace_to_file.cc:68: const char* str = "]}";
On 2014/09/24 23:47:30, Nico (hiding) wrote:
> ditto

Done.

[enne (OOO), 2014-09-24 23:59:11]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:00:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/542893002/140001

[commit-bot: I haz the power, 2014-09-25 00:45:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:45:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-09-25 00:54:50]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:56:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/542893002/160001

[commit-bot: I haz the power, 2014-09-25 01:43:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 01:43:49]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/65344)

[mithro-old, 2014-09-25 01:47:01]

On 2014/09/24 23:47:30, Nico (hiding) wrote:
> I'm away starting tomorrow :-)
> 
> Seems useful, lgtm. It depends on destructors running at test end though,
which
> might not always be true (it currently is)

LGTM

Just a FYI, unless we are doing something special, gtest and gmock both require
this to be true. Thus unless we move to another framework, this should continue
to be true.

Looking forward to this functionality!

[enne (OOO), 2014-09-25 16:58:41]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 16:59:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/542893002/180001

[enne (OOO), 2014-09-25 17:00:17]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-09-25 17:05:47]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/542893002/200001

[commit-bot: I haz the power, 2014-09-25 19:50:04]

Committed patchset #11 (id:200001) as 0f64f38c8c6bf317dc39a6ef88551396904dfd9d

[commit-bot: I haz the power, 2014-09-25 19:50:41]

Patchset 11 (id:??) landed as
https://crrev.com/04213de5a22ff0f95a93584cde042133c358c184
Cr-Commit-Position: refs/heads/master@{#296770}