Add support for writing system traces at startup

Add support for writing system traces at startup

BUG=275822
TEST=Included in  cl
R=dsinclair@chromium.org, sky@chromium.org

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

[dsinclair, 2013-08-19 20:12:25]

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:60: if
(system_tracing_completed_)
Are both of these methods guaranteed to be called from the same thread? If not,
is there a race condition here?

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:62: // TODO(joth): this would
be the time to automatically open up
tracing_completed_ = true;?

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:71: delete this;
system_tracing_completed_ = true?

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
File content/browser/tracing/trace_subscriber_stdio.cc (right):

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_subscriber_stdio.cc:56: const
scoped_refptr<base::RefCountedString>& data_ptr) {
nit: indenting.

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_subscriber_stdio.cc:113: const char* chars =
data.data();
Is it guaranteed that data() won't have any "'s in it? If so, the " will need to
be escaped when written out or it will break the JSON.

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_subscriber_stdio.cc:128: if (IsValid()) {
We seem to be doing if (!IsValid()) early returns in the callers, is this still
needed here?

[DaveMoore, 2013-08-19 22:07:30]

Fix review requests

[DaveMoore, 2013-08-19 22:07:46]

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:60: if
(system_tracing_completed_)
I believe they will both be called on the calling thread.
On 2013/08/19 20:12:26, dsinclair wrote:
> Are both of these methods guaranteed to be called from the same thread? If
not,
> is there a race condition here?

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:62: // TODO(joth): this would
be the time to automatically open up
On 2013/08/19 20:12:26, dsinclair wrote:
> tracing_completed_ = true;?

Done.

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_controller_impl.cc:71: delete this;
On 2013/08/19 20:12:26, dsinclair wrote:
> system_tracing_completed_ = true?

Done.

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
File content/browser/tracing/trace_subscriber_stdio.cc (right):

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_subscriber_stdio.cc:56: const
scoped_refptr<base::RefCountedString>& data_ptr) {
On 2013/08/19 20:12:26, dsinclair wrote:
> nit: indenting.

Done.

https://codereview.chromium.org/23125009/diff/1/content/browser/tracing/trace...
content/browser/tracing/trace_subscriber_stdio.cc:113: const char* chars =
data.data();
On 2013/08/19 20:12:26, dsinclair wrote:
> Is it guaranteed that data() won't have any "'s in it? If so, the " will need
to
> be escaped when written out or it will break the JSON.

Done.

[DaveMoore, 2013-08-19 22:26:47]

Fix call in Android

[DaveMoore, 2013-08-20 13:57:26]

Wait for complete system tracing implementation

[DaveMoore, 2013-08-20 13:58:35]

There's a more complete implementation of chromeos system tracing coming in a
separate cl. This part snuck in accidentally.

[dsinclair, 2013-08-20 14:05:25]

lgtm with one nit.

https://codereview.chromium.org/23125009/diff/58001/content/browser/tracing/t...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/23125009/diff/58001/content/browser/tracing/t...
content/browser/tracing/trace_controller_impl.cc:20: #endif
Is this part of the future cl as well?

[DaveMoore, 2013-08-20 14:07:43]

Wait for complete system tracing implementation

[sky, 2013-08-20 14:28:43]

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.cc (right):

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:20: explicit
TraceSubscriberStdioWorker(const base::FilePath& path,
no explicit

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:51:
WriteString(data_ptr->data());
How come you don't need to escape this?

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:114: std::string data =
data_ptr->data();
const std::string&

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:117: int old_index = 0;
Shouldn't this be a size_t too?

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:134: if (IsValid()) {
Early out if size == 0? Can happen on 128 new.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:148: FileType file_type_;
const on these two (I think).

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:156: };
DISALLOW_COPY...

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.h (right):

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:24: enum FileType {
Please add a description of what these mean.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:31: bool has_system_trace);
Add a description for has_system_trace.

[sky, 2013-08-20 14:36:09]

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.h (right):

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:39: virtual void
OnEndSystemTracing(
One more. Is this part of TraceSubscriber? If so, remove 38 and add OVERRIDE. If
not, does it need to be public and virtual? Also, please document it (assuming
it isn't an OVERRIDE).

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:40: const
scoped_refptr<base::RefCountedString>& events_str_ptr);
nit: two more spaces here.

[DaveMoore, 2013-08-20 14:57:31]

Address review concerns

[DaveMoore, 2013-08-20 14:57:59]

https://codereview.chromium.org/23125009/diff/58001/content/browser/tracing/t...
File content/browser/tracing/trace_controller_impl.cc (right):

https://codereview.chromium.org/23125009/diff/58001/content/browser/tracing/t...
content/browser/tracing/trace_controller_impl.cc:20: #endif
On 2013/08/20 14:05:26, dsinclair wrote:
> Is this part of the future cl as well?

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.cc (right):

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:20: explicit
TraceSubscriberStdioWorker(const base::FilePath& path,
On 2013/08/20 14:28:44, sky wrote:
> no explicit

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:51:
WriteString(data_ptr->data());
It wasn't escaped in the previous implementation. There's no change here from
before, other than it can optionally be put in a property list. I'll research if
this was incorrect and if so do another cl to fix it.

On 2013/08/20 14:28:44, sky wrote:
> How come you don't need to escape this?

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:114: std::string data =
data_ptr->data();
On 2013/08/20 14:28:44, sky wrote:
> const std::string&

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:117: int old_index = 0;
On 2013/08/20 14:28:44, sky wrote:
> Shouldn't this be a size_t too?

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:134: if (IsValid()) {
On 2013/08/20 14:28:44, sky wrote:
> Early out if size == 0? Can happen on 128 new.

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.cc:156: };
On 2013/08/20 14:28:44, sky wrote:
> DISALLOW_COPY...

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.h (right):

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:24: enum FileType {
On 2013/08/20 14:28:44, sky wrote:
> Please add a description of what these mean.

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:31: bool has_system_trace);
On 2013/08/20 14:28:44, sky wrote:
> Add a description for has_system_trace.

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:39: virtual void
OnEndSystemTracing(
On 2013/08/20 14:36:09, sky wrote:
> One more. Is this part of TraceSubscriber? If so, remove 38 and add OVERRIDE.
If
> not, does it need to be public and virtual? Also, please document it (assuming
> it isn't an OVERRIDE).

Done.

https://codereview.chromium.org/23125009/diff/66001/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:40: const
scoped_refptr<base::RefCountedString>& events_str_ptr);
On 2013/08/20 14:36:09, sky wrote:
> nit: two more spaces here.

Done.

[sky, 2013-08-20 15:38:07]

LGTM

https://codereview.chromium.org/23125009/diff/54002/content/browser/tracing/t...
File content/browser/tracing/trace_subscriber_stdio.h (right):

https://codereview.chromium.org/23125009/diff/54002/content/browser/tracing/t...
content/browser/tracing/trace_subscriber_stdio.h:46: virtual void
OnEndSystemTracing(
Do you need the virtual?

[DaveMoore, 2013-08-20 15:42:04]

On 2013/08/20 15:38:07, sky wrote:
> LGTM
> 
>
https://codereview.chromium.org/23125009/diff/54002/content/browser/tracing/t...
> File content/browser/tracing/trace_subscriber_stdio.h (right):
> 
>
https://codereview.chromium.org/23125009/diff/54002/content/browser/tracing/t...
> content/browser/tracing/trace_subscriber_stdio.h:46: virtual void
> OnEndSystemTracing(
> Do you need the virtual?

Yes. It's going to be overridden by the AutoStop implementation

[DaveMoore, 2013-08-20 15:42:58]

Committed patchset #6 manually as r218498 (presubmit successful).