Add TRACE_COUNTER support to about:tracing

Add TRACE_COUNTER support to about:tracing with dramatically improved test coverage.

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

[James Hawkins, 2011-11-16 17:41:38]

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/tests/nonnested_trace.json (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/nonnested_trace.json:2: {"cat": "PERF",
"pid": 22630, "tid": 22630, "ts": 826, "ph": "B",
I'm not sure if we have a style for JSON per se, but if anything, it should
match our JS style.  That means 2 space indentation for blocks and 4 space
indentation for line wraps.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/nonnested_trace.json:23: ]
Indentation off here.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/tests/simple_trace.json (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/simple_trace.json:2: {"cat": "PERF",
"pid": 22630, "tid": 22630, "ts": 826, "ph": "C", "name": "counter", "args":
{"value": 10}},
80 cols.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.css:24: .timeline-canvas-based-track {
What canvas are you referring to?

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:35: throw 'parentEl must be
provided';
In general we don't check parameters like this.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:44: this.onResizeBoundToThis_ =
this.onResize_.bind(this);
Hmm I'm not a fan of this construct.  You lose readability and it's not any
shorter.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:65: 
Remove extra blank line.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:79: cr.dispatchSimpleEvent(this,
'change');
You have this as fireChanged() below; probably good to use that construct here.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:100: this.pendingSetFunction_();
Are you expecting this to fail in some way?

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:277: for (var i = 0; i <
this.tracks_.children.length; ++i)
JS style is i++.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:204: var N = this.numSeries;
s/N/n/

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:260: getOrCreateCounter:
function(name) {
Document this method.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:286: const numColorIds = 30;
Document this var.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline_test.html (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:4: Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2011.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:10: <style>
Move to a CSS file.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:35: function get(url, cb) {
Move to a JS file.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:79: function load(parentEl)
{
Move to a JS file.

[nduca, 2011-11-16 19:03:56]

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/tests/nonnested_trace.json (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/nonnested_trace.json:2: {"cat": "PERF",
"pid": 22630, "tid": 22630, "ts": 826, "ph": "B",
On 2011/11/16 17:41:38, James Hawkins wrote:
> I'm not sure if we have a style for JSON per se, but if anything, it should
> match our JS style.  That means 2 space indentation for blocks and 4 space
> indentation for line wraps.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/nonnested_trace.json:23: ]
On 2011/11/16 17:41:38, James Hawkins wrote:
> Indentation off here.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/tests/simple_trace.json (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/tests/simple_trace.json:2: {"cat": "PERF",
"pid": 22630, "tid": 22630, "ts": 826, "ph": "C", "name": "counter", "args":
{"value": 10}},
On 2011/11/16 17:41:38, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.css:24: .timeline-canvas-based-track {
On 2011/11/16 17:41:38, James Hawkins wrote:
> What canvas are you referring to?

There is a TimelineCanvasBasedTrack class introduced in this patch that is the
base class for TimelineSliceTrack and TimelineCounterTrack.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:35: throw 'parentEl must be
provided';
On 2011/11/16 17:41:38, James Hawkins wrote:
> In general we don't check parameters like this.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:44: this.onResizeBoundToThis_ =
this.onResize_.bind(this);
On 2011/11/16 17:41:38, James Hawkins wrote:
> Hmm I'm not a fan of this construct.  You lose readability and it's not any
> shorter.

Sorry, what's the alternative? I need to remove the listener down the road...

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:65: 
On 2011/11/16 17:41:38, James Hawkins wrote:
> Remove extra blank line.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:79: cr.dispatchSimpleEvent(this,
'change');
On 2011/11/16 17:41:38, James Hawkins wrote:
> You have this as fireChanged() below; probably good to use that construct
here.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:100: this.pendingSetFunction_();
On 2011/11/16 17:41:38, James Hawkins wrote:
> Are you expecting this to fail in some way?

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline.js:277: for (var i = 0; i <
this.tracks_.children.length; ++i)
On 2011/11/16 17:41:38, James Hawkins wrote:
> JS style is i++.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:204: var N = this.numSeries;
On 2011/11/16 17:41:38, James Hawkins wrote:
> s/N/n/

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:260: getOrCreateCounter:
function(name) {
On 2011/11/16 17:41:38, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_model.js:286: const numColorIds = 30;
On 2011/11/16 17:41:38, James Hawkins wrote:
> Document this var.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
File chrome/browser/resources/tracing/timeline_test.html (right):

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:4: Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/11/16 17:41:38, James Hawkins wrote:
> 2011.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:10: <style>
On 2011/11/16 17:41:38, James Hawkins wrote:
> Move to a CSS file.
Killed this entirely. It was just convenience stuff anyway.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:35: function get(url, cb) {
On 2011/11/16 17:41:38, James Hawkins wrote:
> Move to a JS file.

Done.

http://codereview.chromium.org/8513009/diff/1/chrome/browser/resources/tracin...
chrome/browser/resources/tracing/timeline_test.html:79: function load(parentEl)
{
On 2011/11/16 17:41:38, James Hawkins wrote:
> Move to a JS file.

This code is specific to this test group. I was copying from the closure-style
tests where you put the test code right in html... thoughts?

[jbates, 2011-11-16 23:25:39]

lgtm

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:384: * whether whether to respond
to keyboard inputs.
whether whether -> whether

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_model.js:457:
.getOrCreateCounter(event.name);
Do we care that ("cat1", "name1") is the same counter as ("cat2", "name1")?

[James Hawkins, 2011-11-17 05:19:31]

LGTM with nits.

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/tests/simple_trace.json (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/tests/simple_trace.json:5: "name": "A long name
that doesnt fit but is exceedingly informative", "args": {}},
80 cols.

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:79: checkForAttach_: function() {
Document method.

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_track_test.html (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_track_test.html:10: <script
src="http://closure-library.googlecode.com/svn/trunk/closure/goog/base.js"></script>
80 cols.

[nduca, 2011-11-18 08:58:19]

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/tests/simple_trace.json (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/tests/simple_trace.json:5: "name": "A long name
that doesnt fit but is exceedingly informative", "args": {}},
On 2011/11/17 05:19:32, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8513009/diff/6001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_model.js:457:
.getOrCreateCounter(event.name);
ooo great point.
On 2011/11/16 23:25:39, jbates wrote:
> Do we care that ("cat1", "name1") is the same counter as ("cat2", "name1")?