Support JSON encoding of data for about:tracking information

Support JSON encoding of data for about:tracking information

I also fought a terrible (but educational) fight with Thread Local Store, and its ability to do cleanup (call destructors) at thread exit (notably applicable to Worker Threads). Thta is why there were soooo many test bot runs and tiny checkins. I now have a plan in mind that won't rely on that functionality.  The code seems to work cross-platform, but if I have trouble with Linux, I'll repeatedly leak ThreadData contexts temporarily on that platform.  Given that the code is only enabled under Debug, this is not yet a real problem.

Each CL I write for this code also includes a bunch of cleanup.  In this case, I changed the Write() methods to WriteHTML(), since I didn't want any confusion with JSON writing etc.  I also did a bunch of tiny cleanups which should not have  changed what the code does.

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

[jar (doing other things), 2011-10-24 03:40:17]

Eric and/or Raman,

I'm pretty sure in addition to providing the ToValue() code (the external API is
the call to the static method ThreadData::ToValue()) I've tamed the ThreadLocal
store problems (surrounding cleanup as worker threads come and go). The try bots
were down today, so I couldn't get proof positive yet, but I'd like one of you
to look at the code and possibly given it a go for landing (subject to trybot
green-ness).

Thanks in advance,

Jim

[ramant (doing other things), 2011-10-24 18:03:48]

LGTM. small nits.

http://codereview.chromium.org/8313013/diff/27025/base/location.cc
File base/location.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/location.cc#newcode76
base/location.cc:76: dictionary->Set("file_name",
base::Value::CreateStringValue(file_name_ ));
nit: extra space after "file_name_".

http://codereview.chromium.org/8313013/diff/27025/base/location.cc#newcode81
base/location.cc:81: base::Value::CreateStringValue(function_name_ ));
nit: extra space after "function_name_"

http://codereview.chromium.org/8313013/diff/27025/base/location.h
File base/location.h (right):

http://codereview.chromium.org/8313013/diff/27025/base/location.h#newcode58
base/location.h:58: // string with HTML characters in the function names
exscaped, and append
nit:  exscaped ->  escaped

http://codereview.chromium.org/8313013/diff/27025/base/location.h#newcode67
base/location.h:67: // Construct a Value* representation.  The caller ownns
assumes ownership
nit: ownns -> owns. Should we say owns and assumes?

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.cc#new...
base/tracked_objects.cc:43: int ThreadData::thread_number_counter = 0;
nit: g_thread_number_counter

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:11: #include <stack>
nit: "#include <stack>" should come before #include <string>

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:538: // The |end_of_run| is was just obtained by a call
to Now() (just after the
nit: "is was" -> should we say was?

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:692: // all ollocated data structures).  As a result, we
don't have any code in this
nit: allocated

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
File base/tracked_objects_unittest.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:19: 
nit: lint was saying no extra space after public:

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:102:
ThreadData::ShutdownSingleThreadedCleanup();
nit: consider putting it in TearDown?

[jar (doing other things), 2011-10-24 18:43:27]

Changes in response to Rtenneti's review.

All the try bots are passing (or at least were before this last round of mostly
nits).  Assuming I get more green from them, I'll be landing shortly.

http://codereview.chromium.org/8313013/diff/27025/base/location.cc
File base/location.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/location.cc#newcode76
base/location.cc:76: dictionary->Set("file_name",
base::Value::CreateStringValue(file_name_ ));
On 2011/10/24 18:03:49, ramant wrote:
> nit: extra space after "file_name_".

Done.

http://codereview.chromium.org/8313013/diff/27025/base/location.cc#newcode81
base/location.cc:81: base::Value::CreateStringValue(function_name_ ));
On 2011/10/24 18:03:49, ramant wrote:
> nit: extra space after "function_name_"

Done.

http://codereview.chromium.org/8313013/diff/27025/base/location.h
File base/location.h (right):

http://codereview.chromium.org/8313013/diff/27025/base/location.h#newcode58
base/location.h:58: // string with HTML characters in the function names
exscaped, and append
On 2011/10/24 18:03:49, ramant wrote:
> nit:  exscaped ->  escaped

Done.

http://codereview.chromium.org/8313013/diff/27025/base/location.h#newcode67
base/location.h:67: // Construct a Value* representation.  The caller ownns
assumes ownership
On 2011/10/24 18:03:49, ramant wrote:
> nit: ownns -> owns. Should we say owns and assumes?

Done.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.cc
File base/tracked_objects.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.cc#new...
base/tracked_objects.cc:43: int ThreadData::thread_number_counter = 0;
On 2011/10/24 18:03:49, ramant wrote:
> nit: g_thread_number_counter

Done.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:11: #include <stack>
On 2011/10/24 18:03:49, ramant wrote:
> nit: "#include <stack>" should come before #include <string>

Done.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:538: // The |end_of_run| is was just obtained by a call
to Now() (just after the
On 2011/10/24 18:03:49, ramant wrote:
> nit: "is was" -> should we say was?

Done.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects.h#newc...
base/tracked_objects.h:692: // all ollocated data structures).  As a result, we
don't have any code in this
On 2011/10/24 18:03:49, ramant wrote:
> nit: allocated

Done.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
File base/tracked_objects_unittest.cc (right):

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:19: 
On 2011/10/24 18:03:49, ramant wrote:
> nit: lint was saying no extra space after public:

Removed references to message_loop (including header inclusion), and added a
tear-down destructor.

http://codereview.chromium.org/8313013/diff/27025/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:102:
ThreadData::ShutdownSingleThreadedCleanup();
On 2011/10/24 18:03:49, ramant wrote:
> nit: consider putting it in TearDown?

Done.

[eroman, 2011-10-24 22:23:03]

A couple style nits (didn't review this in detail, consider Raman to be your
main reviewer)

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:253: // Constructe a DictionaryValue instance containing
all our stats. The caller
typo: construct

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:342: // Generate a ListValue representation of the vector
of snapshots.  The caller
nit: style guide says to use passive tense for function comments, as in
"Generates".

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:518: // Constructe a ListValue instance containing all
recursive results in our
typo construct.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:522: static base::Value* ToValue(int process_type);
not sure about this process_type parameter.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
File base/tracked_objects_unittest.cc (right):

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:18: ~TrackedObjectsTest() {
nit: indentation

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:173: "\"birth_thread\":\"WorkerThread-1\","
Are thread names guaranteed to be unique? I wander if we should track the thread
by ID, and then have an auxiliary mapping from ID --> Name.

[jar (doing other things), 2011-10-27 06:23:34]

Comments in response to Eroman.

I landed this CL before reading your suggestions, so I have put all your
suggestions into the next CL (which will arrive later today or tomorrow??).  

Jim

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h
File base/tracked_objects.h (right):

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:253: // Constructe a DictionaryValue instance containing
all our stats. The caller
On 2011/10/24 22:23:04, eroman wrote:
> typo: construct

Done.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:342: // Generate a ListValue representation of the vector
of snapshots.  The caller
On 2011/10/24 22:23:04, eroman wrote:
> nit: style guide says to use passive tense for function comments, as in
> "Generates".

Done.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:518: // Constructe a ListValue instance containing all
recursive results in our
On 2011/10/24 22:23:04, eroman wrote:
> typo construct.

Done.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects.h#newc...
base/tracked_objects.h:522: static base::Value* ToValue(int process_type);
On 2011/10/24 22:23:04, eroman wrote:
> not sure about this process_type parameter.

Rephrased as a TODO task.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
File base/tracked_objects_unittest.cc (right):

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:18: ~TrackedObjectsTest() {
On 2011/10/24 22:23:04, eroman wrote:
> nit: indentation

Done.

http://codereview.chromium.org/8313013/diff/32005/base/tracked_objects_unitte...
base/tracked_objects_unittest.cc:173: "\"birth_thread\":\"WorkerThread-1\","
It is possible (within a single process) to have two threads named idenically. 
Currently, the named threads all have unique thread-names, and I manually name
the WorkerThreads with unique names.

When we go cross-process (and get multiple renders, in addition to the browser),
I'm sure we'll have dups... but I think we'd have dups based on ThreadId as
well.

Bottom line: I don't this this is a problem at this point.

On 2011/10/24 22:23:04, eroman wrote:
> Are thread names guaranteed to be unique? I wander if we should track the
thread
> by ID, and then have an auxiliary mapping from ID --> Name.