Allow TracingController to output trace data to specified file

Allow TracingController output trace data to specified file

Sometimes we need to let TracingController output traces to specified
file, e.g. trace-startup (which will be migrated from TraceController
to TracingController).

Also fixed the problem of IO operations in UI thread.

BUG=none
TEST=TracingControllerTest.EnableAndDisableRecordingWithFilePath
TEST=TracingControllerTest.EnableCaptureAndDisableMonitoringWithFilePath
R=joi@chromium.org, nduca@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236071

[Xianzhu, 2013-11-12 16:38:45]

ping...

[Xianzhu, 2013-11-13 17:01:19]

nduca@, as dsinclair@ is on vacation, could you have a look at this CL (and also
https://codereview.chromium.org/66193005/ and
https://codereview.chromium.org/65343006/). This is my first step to implement
tracing in about:inspect (to remove TraceController).

[nduca, 2013-11-15 23:12:25]

lgtm

[dsinclair, 2013-11-18 16:32:18]

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:55: bool
has_at_least_one_result_;
Should this be DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:77: size_t written =
fwrite(preamble, strlen(preamble), 1, file_);
This (and the DCHECK) are a little weird, we aren't writing 1 element of length
strlen(preamble) we're writing strlen(preamble) items of length 1.

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:91: has_at_least_one_result_
= true;
Don't really need the else as we can always just set has_at_least_one_result to
true.

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:93:
events_str_ptr->data().size(), 1,
As above, params seem backwards.

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
content/public/browser/tracing_controller.h:82: // If |result_file_path| is
empty and |callback| is null, trace data won't be
Isn't this an OR instead of an AND.

[Xianzhu, 2013-11-18 21:50:38]

And +joi@ for content/public/browser/tracing_controller.h.

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:55: bool
has_at_least_one_result_;
On 2013/11/18 16:32:18, dsinclair wrote:
> Should this be DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:77: size_t written =
fwrite(preamble, strlen(preamble), 1, file_);
On 2013/11/18 16:32:18, dsinclair wrote:
> This (and the DCHECK) are a little weird, we aren't writing 1 element of
length
> strlen(preamble) we're writing strlen(preamble) items of length 1.

This is original code moved here. According to the sequence of parameters, it
does write 1 element of length strlen(preable). Compared to write
strlen(preable) items of length 1, the current code is simpler in the checking
(otherwise we'd need to save strlen(preable) or repeat it).

https://codereview.chromium.org/66893003/diff/180001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:91: has_at_least_one_result_
= true;
On 2013/11/18 16:32:18, dsinclair wrote:
> Don't really need the else as we can always just set has_at_least_one_result
to
> true.

Done.

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
content/public/browser/tracing_controller.h:82: // If |result_file_path| is
empty and |callback| is null, trace data won't be
On 2013/11/18 16:32:18, dsinclair wrote:
> Isn't this an OR instead of an AND.

Used 'AND' because I thought the following might be a useful case: the caller
wants to save the traces into a file but don't care when the async process
finishes. The current impl is following the comment here.

[Xianzhu, 2013-11-18 21:56:09]

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/66893003/diff/180001/content/public/browser/t...
content/public/browser/tracing_controller.h:82: // If |result_file_path| is
empty and |callback| is null, trace data won't be
On 2013/11/18 21:50:39, Xianzhu wrote:
> On 2013/11/18 16:32:18, dsinclair wrote:
> > Isn't this an OR instead of an AND.
> 
> Used 'AND' because I thought the following might be a useful case: the caller
> wants to save the traces into a file but don't care when the async process
> finishes. The current impl is following the comment here.

Note this is also consistent with other methods that will do the actual thing
(if it's meaningful) when the callback is null.

[Jói, 2013-11-19 12:42:22]

https://codereview.chromium.org/66893003/diff/350001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/66893003/diff/350001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:196: result_file_.reset(new
ResultFile(result_file_path));
Do you not need to close the previous result_file_ before setting it?

https://codereview.chromium.org/66893003/diff/350001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/66893003/diff/350001/content/public/browser/t...
content/public/browser/tracing_controller.h:80: // into a temporary file. The
actual file path will be passed to |callback| if
into a temporary file -> into the previously set file (?)

https://codereview.chromium.org/66893003/diff/350001/content/public/browser/t...
content/public/browser/tracing_controller.h:83: // If |result_file_path| is
empty and |callback| is null, trace data won't be
This doesn't look like it's quite accurate for this function; result_file_ is
reset only if either |callback| is not null or |result_file_path| is not empty.
In other cases it will be written to |result_file_path| or a temp file if that
is empty.

[Xianzhu, 2013-11-19 16:48:34]

PTAL.

https://codereview.chromium.org/66893003/diff/350001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/66893003/diff/350001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:196: result_file_.reset(new
ResultFile(result_file_path));
On 2013/11/19 12:42:22, Jói wrote:
> Do you not need to close the previous result_file_ before setting it?

The !can_disable_recording() condition above ensures that an open ResultFile
won't be replaced here. result_file_ should be either initially NULL or has been
reset in the last OnResultFileClosed.

Modified can_disable_recording() to make the logic more explicit.

https://codereview.chromium.org/66893003/diff/350001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/66893003/diff/350001/content/public/browser/t...
content/public/browser/tracing_controller.h:83: // If |result_file_path| is
empty and |callback| is null, trace data won't be
On 2013/11/19 12:42:22, Jói wrote:
> This doesn't look like it's quite accurate for this function; result_file_ is
> reset only if either |callback| is not null or |result_file_path| is not
empty.
> In other cases it will be written to |result_file_path| or a temp file if that
> is empty.

result_file_ will be reset to NULL when the whole DisableRecording() process
finishes (in OnResultFileClosed). DisableRecording() will return false directly
if there is no recording or the last DisableRecording() has not finished. So
every time DisableRecording() doesn't return false, ResultFile is always a new
object and will create temporary file if |result_file_path| is empty.

Changed can_disable_recording to make the logic more explicit.

[Jói, 2013-11-19 16:50:48]

LGTM

[commit-bot: I haz the power, 2013-11-19 18:23:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/66893003/430001

[commit-bot: I haz the power, 2013-11-19 20:51:40]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-19 21:06:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/66893003/430001

[Xianzhu, 2013-11-19 23:52:41]

Committed patchset #9 manually as r236071 (presubmit successful).