Internalize JSON chunk management to trace_event.h API.

Internalize JSON chunk merging to trace_event.h API.

BUG=100291
TEST=base_unittests, content_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106808

[jbates, 2011-10-20 00:05:00]

nduca: please review.
joth: please check the trace_subscriber_stdio bits.
darin: please rubber stamp (sorry I keep throwing reviews at you! you are just
one of two owners that covers all these paths... and the other is on paternity
leave.)

[nduca, 2011-10-20 00:28:03]

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.cc#newco...
base/debug/trace_event.cc:279: for (size_t i = 0; i < fragments_.size(); ++i) {
Algorithmically, I see a bit too many strdups. I'd <3 to see this do:

std::string result_; bool terminated_;
constructor: result_("["), terminated_(false);
addfragment: DCHECK(!terminated_); result_ += trace_fragment; result_ += ",";
getJSON() {
  if(!terminated_) {
    terminated_ = true;
    result += "]";
  }
  return result_;
}

bonus points for unterminating instead of dchecking.

Less strduping == good, yes?

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h#newcod...
base/debug/trace_event.h:526: void AddFragment(const std::string&
trace_fragment);
Brownie points: typedef TraceFragment so nobody sees the type except
TraceResultBuffer?

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event_unittest.cc
File base/debug/trace_event_unittest.cc (left):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event_unittest...
base/debug/trace_event_unittest.cc:726: 
I can haz a unit test for the trace buffer?

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
chrome/browser/ui/webui/tracing_ui.cc:424: base::debug::TraceResultBuffer
trace_buffer_;
I wasn't expecting this. Was expecting the trace_buffer to be a member on the
message handler, and for us to only hand the the string to JS at the end. I
realize that might then blow-out JS' string size limits, but it'd be
better-matched to the design.

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:47: size_t written =
fwrite(json_trace_data.c_str(), 1, json_trace_data.size(),
/me worries about this blowing out memory on clank. The reason this writes to
file is because of the memory limit.

Would it be pushing your sense of humor to do a second implementation which is
streaming? We'd use that in tracing_ui and here...

TraceFragmentConverter {
   Callback1 <string> callback_;
   constructor: callback("[");
   destructor: callback("]");
   AddFragment(frag) { callback_(fragment + ","); }
}

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio_unittest.cc (left):

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio_unittest.cc:40: EXPECT_EQ("[foo,bar,]",
ReadTraceFile());
why'd this get deleted? merge issue?

[jbates, 2011-10-20 22:18:49]

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.cc
File base/debug/trace_event.cc (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.cc#newco...
base/debug/trace_event.cc:279: for (size_t i = 0; i < fragments_.size(); ++i) {
On 2011/10/20 00:28:03, nduca wrote:
> Algorithmically, I see a bit too many strdups. I'd <3 to see this do:
> 
> std::string result_; bool terminated_;
> constructor: result_("["), terminated_(false);
> addfragment: DCHECK(!terminated_); result_ += trace_fragment; result_ += ",";
> getJSON() {
>   if(!terminated_) {
>     terminated_ = true;
>     result += "]";
>   }
>   return result_;
> }
> 
> bonus points for unterminating instead of dchecking.
> 
> Less strduping == good, yes?

Similar optimizations occurred to me, but I thought performance was not
important here so not worth the design complication / code maintenance. Ah well!
Done.

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h#newcod...
base/debug/trace_event.h:526: void AddFragment(const std::string&
trace_fragment);
On 2011/10/20 00:28:03, nduca wrote:
> Brownie points: typedef TraceFragment so nobody sees the type except
> TraceResultBuffer? 

I wanted to do this as well, but I didn't want to add the IPC struct Traits code
in this CL.

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event_unittest.cc
File base/debug/trace_event_unittest.cc (left):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event_unittest...
base/debug/trace_event_unittest.cc:726: 
On 2011/10/20 00:28:03, nduca wrote:
> I can haz a unit test for the trace buffer?

Done.

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
chrome/browser/ui/webui/tracing_ui.cc:424: base::debug::TraceResultBuffer
trace_buffer_;
On 2011/10/20 00:28:03, nduca wrote:
> I wasn't expecting this. Was expecting the trace_buffer to be a member on the
> message handler, and for us to only hand the the string to JS at the end. I
> realize that might then blow-out JS' string size limits, but it'd be
> better-matched to the design.

It's better for memory performance to pass these through, so let's keep it as
is.

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:47: size_t written =
fwrite(json_trace_data.c_str(), 1, json_trace_data.size(),
On 2011/10/20 00:28:03, nduca wrote:
> /me worries about this blowing out memory on clank. The reason this writes to
> file is because of the memory limit.
> 
> Would it be pushing your sense of humor to do a second implementation which is
> streaming? We'd use that in tracing_ui and here...
> 
> TraceFragmentConverter {
>    Callback1 <string> callback_;
>    constructor: callback("[");
>    destructor: callback("]");
>    AddFragment(frag) { callback_(fragment + ","); }
> }

Done.

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio_unittest.cc (left):

http://codereview.chromium.org/8355024/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio_unittest.cc:40: EXPECT_EQ("[foo,bar,]",
ReadTraceFile());
On 2011/10/20 00:28:03, nduca wrote:
> why'd this get deleted? merge issue?

This isn't the place to test the bracket formatting now that we do it inside
trace_event.cc -- this test is now in trace_event_unittests.

[nduca, 2011-10-20 23:20:05]

LGTM if you're in a hurry. Lots of nits.

In particular, can you new.crbug using the output callback in the Tracing UI and
assign to me if you're not wanting to do it?

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h#newcod...
base/debug/trace_event.h:526: void AddFragment(const std::string&
trace_fragment);
I realize its work, but we both agree its the right thing to do.

I'd love to pep-talk you into doing this, because I'm not gonna insist on it
from a procedural point of view. Its just silly to file a crbug for this, and it
smells funny.

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
chrome/browser/ui/webui/tracing_ui.cc:424: base::debug::TraceResultBuffer
trace_buffer_;
Now that you have an output callback, use that instead. E.g. have a
trace_buffer_, and route the output callback to the ExecuteJavascript. You'll
have to tweak onTraceDataCollected to use strings instead of json objects, but
grab me and I can help you.

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:946: Clear();
This is two tests, no?

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:953: 
Output callback test?
Any corner cases?

Not a big deal if you're feeling overwhelmed.

[jbates, 2011-10-20 23:55:12]

darin, joth: PTAL

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8355024/diff/1/base/debug/trace_event.h#newcod...
base/debug/trace_event.h:526: void AddFragment(const std::string&
trace_fragment);
On 2011/10/20 23:20:05, nduca wrote:
> I realize its work, but we both agree its the right thing to do.
> 
> I'd love to pep-talk you into doing this, because I'm not gonna insist on it
> from a procedural point of view. Its just silly to file a crbug for this, and
it
> smells funny.

Yup, we agree it's a good design, but low priority compared to the other issues
on my plate.

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/8355024/diff/1/chrome/browser/ui/webui/tracing...
chrome/browser/ui/webui/tracing_ui.cc:424: base::debug::TraceResultBuffer
trace_buffer_;
On 2011/10/20 23:20:05, nduca wrote:
> Now that you have an output callback, use that instead. E.g. have a
> trace_buffer_, and route the output callback to the ExecuteJavascript. You'll
> have to tweak onTraceDataCollected to use strings instead of json objects, but
> grab me and I can help you.

Since you've spent more time in the javascript side of this system, perhaps this
is a quick followup patch for you? The current code looks clean to me, but maybe
that's because I'm not familiar with the JS-side subtleties that you are
thinking about...

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
File base/debug/trace_event_unittest.cc (right):

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:946: Clear();
On 2011/10/20 23:20:05, nduca wrote:
> This is two tests, no?

Easier to follow in one test IMO. Is there a rule about this that I don't know?

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event_unitt...
base/debug/trace_event_unittest.cc:953: 
On 2011/10/20 23:20:05, nduca wrote:
> Output callback test?
> Any corner cases?

Output callback is tested by every other test now, so I think it's covered.

[jbates, 2011-10-21 21:11:02]

jam: please rubber stamp
(removed darin because he is on vacation)

[jam, 2011-10-21 21:28:23]

lgtm

[joth, 2011-10-24 08:09:29]

LGTM (stdio subscriber)

Nuts - I reviewed this on Friday morning but never hit send :-( but looks like
you've addressed all but one of my comments anyway.... :)
Thanks!

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

http://codereview.chromium.org/8355024/diff/6001/base/debug/trace_event.h#new...
base/debug/trace_event.h:520: class TraceResultBuffer {
BASE_EXPORT ?