TraceSubscriber implementation that writes to a file.

TraceSubscriber implementation that writes to a file.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87459

[lain Merrick, 2011-05-18 17:23:41]

Simple stdio-based TraceSubscriber implementation. Still needs to be hooked up
to a command-line flag and/or UI control -- I'm leaving that for a future CL.

[nduca, 2011-05-18 17:37:07]

LGTM. Thanks Iain!
On 2011/05/18 17:23:41, Iain Merrick wrote:
> Simple stdio-based TraceSubscriber implementation. Still needs to be hooked up
> to a command-line flag and/or UI control -- I'm leaving that for a future CL.

[jbates, 2011-05-18 18:37:56]

LGTM

[Paweł Hajdan Jr., 2011-05-18 20:00:46]

Drive-by with minor testing comments (chrome/test/OWNERS).

Please let me take another look before committing. It's common for people to
misuse even ScopedTempDir.

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:17:
ASSERT_TRUE(file_util::CreateNewTempDirectory(kFolderPrefix, &trace_dir_));
Oh, could you just use ScopedTempDir instead?

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:28:
file_util::ReadFileToString(trace_file_, &result);
How about checking the return value?

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:48: }
nit: Add "  // namespace"

[lain Merrick, 2011-05-19 15:49:00]

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:17:
ASSERT_TRUE(file_util::CreateNewTempDirectory(kFolderPrefix, &trace_dir_));
On 2011/05/18 20:00:46, Paweł Hajdan Jr. wrote:
> Oh, could you just use ScopedTempDir instead?

Done.

I originally based this code on some existing tests which used FileAutoDeleter,
but then realised that's in chrome/test and we probably shouldn't include it
from content/browser. Maybe we should refactor those tests to use ScopedTempDir,
or move chrome/test/file_test_utils.h into base?

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:28:
file_util::ReadFileToString(trace_file_, &result);
On 2011/05/18 20:00:46, Paweł Hajdan Jr. wrote:
> How about checking the return value?

Done.

http://codereview.chromium.org/7044010/diff/5001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:48: }
On 2011/05/18 20:00:46, Paweł Hajdan Jr. wrote:
> nit: Add "  // namespace"

Done.

[sadrul, 2011-05-19 15:50:48]

Nice! LGTM.

(You will need LGTM from one of the OWNERs for content/, though)

[Paweł Hajdan Jr., 2011-05-19 16:26:49]

Yeah, I think FileAutoDeleter should die in favor of ScopedTempDir. I'd be happy
to review a change to convert users of FileAutoDeleter to ScopedTempDir if you'd
like to do it (up to you).

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:5: #include
"content/browser/trace_subscriber_stdio.h"
nit: Add an empty line below.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:9: #include
"testing/platform_test.h"
nit: This doesn't seem to use PlatformTest.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:21: trace_dir_.reset();
This shouldn't be needed. Please remove entire TearDown then.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:29: scoped_ptr<ScopedTempDir>
trace_dir_;
ScopedTempDir shouldn't need a scoped_ptr. ScopedTempDir trace_dir_ should be
sufficient. The entire test fixture class is created and destroyed for each test
case.

[lain Merrick, 2011-05-19 16:38:02]

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:5: #include
"content/browser/trace_subscriber_stdio.h"
On 2011/05/19 16:26:49, Paweł Hajdan Jr. wrote:
> nit: Add an empty line below.

Done.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:9: #include
"testing/platform_test.h"
On 2011/05/19 16:26:49, Paweł Hajdan Jr. wrote:
> nit: This doesn't seem to use PlatformTest.

Done.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:21: trace_dir_.reset();
On 2011/05/19 16:26:49, Paweł Hajdan Jr. wrote:
> This shouldn't be needed. Please remove entire TearDown then.

Done.

http://codereview.chromium.org/7044010/diff/7001/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:29: scoped_ptr<ScopedTempDir>
trace_dir_;
On 2011/05/19 16:26:49, Paweł Hajdan Jr. wrote:
> ScopedTempDir shouldn't need a scoped_ptr. ScopedTempDir trace_dir_ should be
> sufficient. The entire test fixture class is created and destroyed for each
test
> case.

Done.

[Paweł Hajdan Jr., 2011-05-19 16:45:58]

Code I commented in the drive-by LGTM, thanks!

It's also a bit simpler now, isn't it?

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:7: #include
"base/memory/scoped_ptr.h"
nit: Now scoped_ptr doesn't seem to be used, please remove it.

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:18: }
nit: Add an empty line below.

[lain Merrick, 2011-05-19 16:51:01]

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:7: #include
"base/memory/scoped_ptr.h"
On 2011/05/19 16:45:58, Paweł Hajdan Jr. wrote:
> nit: Now scoped_ptr doesn't seem to be used, please remove it.

Oops, fixed. I should have done another pass myself after addressing your last
set of comments, sorry.

http://codereview.chromium.org/7044010/diff/9004/content/browser/trace_subscr...
content/browser/trace_subscriber_stdio_unittest.cc:18: }
On 2011/05/19 16:45:58, Paweł Hajdan Jr. wrote:
> nit: Add an empty line below.

Done.

[lain Merrick, 2011-05-20 13:42:13]

On 2011/05/19 16:45:58, Paweł Hajdan Jr. wrote:
> Code I commented in the drive-by LGTM, thanks!
> 
> It's also a bit simpler now, isn't it?

Yes, thanks for your help!

I'm a little unclear on whether you meant to LGTM the whole thing there. If so
I'll ping the OWNERS. But no rush, this and 7056011 can go together.

[Paweł Hajdan Jr., 2011-05-20 13:52:09]

On 2011/05/20 13:42:13, Iain Merrick wrote:
> Yes, thanks for your help!

You're welcome!

> I'm a little unclear on whether you meant to LGTM the whole thing there. If so
> I'll ping the OWNERS. But no rush, this and 7056011 can go together.

I only reviewed the test code, and only from a very specific POV. By "code I
commented in the drive-by LGTM" I meant that it only applies to the parts I
commented.

[lain Merrick, 2011-05-24 15:12:42]

Adding Ben for content/OWNERS review.

Ben, do you have time to take a quick look at this?

[lain Merrick, 2011-05-27 14:14:57]

On 2011/05/24 15:12:42, Iain Merrick wrote:
> Adding Ben for content/OWNERS review.

Hi Ben,

Just wanted to check that this CL is on your radar. I know you're probably busy,
and I'm not sure of the etiquette for OWNERS reviews; should I ask a different
owner for the review?

Cheers,

Iain

[lain Merrick, 2011-05-30 08:58:04]

Adding Darin for content/OWNERS review.

[lain Merrick, 2011-05-31 17:47:05]

+jam for content/OWNERS this time. And I just spoke to him on IM so he
definitely knows about this CL. :)

[jam, 2011-05-31 17:49:25]

rubberstamp lgtm

http://codereview.chromium.org/7044010/diff/28001/content/browser/trace_subsc...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/28001/content/browser/trace_subsc...
content/browser/trace_subscriber_stdio_unittest.cc:41: }  // namespace
nit: usually tests aren't put in an anonymous namespace, i.e. this brace bracket
should move to line 29

[lain Merrick, 2011-05-31 18:03:07]

http://codereview.chromium.org/7044010/diff/28001/content/browser/trace_subsc...
File content/browser/trace_subscriber_stdio_unittest.cc (right):

http://codereview.chromium.org/7044010/diff/28001/content/browser/trace_subsc...
content/browser/trace_subscriber_stdio_unittest.cc:41: }  // namespace
On 2011/05/31 17:49:26, John Abd-El-Malek wrote:
> nit: usually tests aren't put in an anonymous namespace, i.e. this brace
bracket
> should move to line 29 

Done.

[jam, 2011-06-22 02:40:49]

http://codereview.chromium.org/7044010/diff/39001/content/browser/trace_subsc...
File content/browser/trace_subscriber_stdio.h (right):

http://codereview.chromium.org/7044010/diff/39001/content/browser/trace_subsc...
content/browser/trace_subscriber_stdio.h:30: FILE* m_file;
hi, i just saw this in another change.  we don't use hungarian notation.  this
should be file_

[lain Merrick, 2011-06-22 10:04:51]

On 2011/06/22 02:40:49, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7044010/diff/39001/content/browser/trace_subsc...
> File content/browser/trace_subscriber_stdio.h (right):
> 
>
http://codereview.chromium.org/7044010/diff/39001/content/browser/trace_subsc...
> content/browser/trace_subscriber_stdio.h:30: FILE* m_file;
> hi, i just saw this in another change.  we don't use hungarian notation.  this
> should be file_

Whoops, sorry about that. I got mixed up between the Chromium and WebKit
conventions.

Fix uploaded as http://codereview.chromium.org/7207005