about:gpu support for thread name metadata.

about:gpu support for thread name metadata.

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

[nduca, 2011-07-26 09:46:32]

http://codereview.chromium.org/7495036/diff/1/chrome/browser/resources/gpu_in...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/1/chrome/browser/resources/gpu_in...
chrome/browser/resources/gpu_internals/timeline_model.js:242:
console.log('Unrecognized event phase: ' + event.ph +
stop throwing errors and switch to logging. Unrecognized event phases aren't
catastrophic to parsing, they just aren't meaningful. This way if someone opens
a file from a newer chrome, or records a trace event that was unexpected, we
don't explode.

http://codereview.chromium.org/7495036/diff/1/chrome/browser/resources/gpu_in...
chrome/browser/resources/gpu_internals/timeline_model.js:287: if
(thread.minTimestamp != undefined &&
Fix a small issue spotted by tonyg for small traces where threads that start at
exactly zero die. (because 0 is falsy)

http://codereview.chromium.org/7495036/diff/1/chrome/browser/resources/gpu_in...
File chrome/browser/resources/gpu_internals/timeline_track.js (right):

http://codereview.chromium.org/7495036/diff/1/chrome/browser/resources/gpu_in...
chrome/browser/resources/gpu_internals/timeline_track.js:219:
this.headingDiv_.textContent = text;
Tooltips ftw!

[jbates, 2011-07-26 18:28:11]

Is it difficult to show process names and keep them in order?

http://code.google.com/p/chromium/issues/detail?id=80326

http://codereview.chromium.org/7495036/diff/3001/chrome/browser/resources/gpu...
File chrome/browser/resources/gpu_internals/timeline_track.js (right):

http://codereview.chromium.org/7495036/diff/3001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_track.js:140: this.thread_.name
? ', name; ' + this.thread_.name : '';
semicolon after name -> colon

[nduca, 2011-07-28 01:46:41]

+jhawkins due to arv out

[James Hawkins, 2011-07-28 21:52:14]

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:68: name: undefined,
Document var. If it's intended to be private, append an underscore.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:273:
console.log('Unrecognized metadata name: ' + event.name);
Remove console spam here and elsewhere.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:314: pruneEmptyThreads:
function() {
Document method.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
File chrome/browser/resources/gpu_internals/tracing_controller_tests.js (right):

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/tracing_controller_tests.js:13:
{"name":"a","args":{},"pid":6052,"ts":3074899524,"cat":"foo","tid":6053,"ph":"B"},
80 cols

[nduca, 2011-07-29 03:16:30]

Thanks James. Left a a few more 80cols issues so jrg can land his 80col changes
without conflict.

[James Hawkins, 2011-07-31 23:59:29]

On 2011/07/29 03:16:30, nduca wrote:
> Thanks James. Left a a few more 80cols issues so jrg can land his 80col
changes
> without conflict.

Please click 'Done' for all of my review comments that you fixed, so I can know
explicitly what you've changed.

[nduca, 2011-08-01 01:22:45]

Er, I did all of them.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:68: name: undefined,
On 2011/07/28 21:52:15, James Hawkins wrote:
> Document var. If it's intended to be private, append an underscore.

Done.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:273:
console.log('Unrecognized metadata name: ' + event.name);
On 2011/07/28 21:52:15, James Hawkins wrote:
> Remove console spam here and elsewhere.

Done.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/timeline_model.js:314: pruneEmptyThreads:
function() {
On 2011/07/28 21:52:15, James Hawkins wrote:
> Document method.

Done.

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
File chrome/browser/resources/gpu_internals/tracing_controller_tests.js (right):

http://codereview.chromium.org/7495036/diff/9001/chrome/browser/resources/gpu...
chrome/browser/resources/gpu_internals/tracing_controller_tests.js:13:
{"name":"a","args":{},"pid":6052,"ts":3074899524,"cat":"foo","tid":6053,"ph":"B"},
On 2011/07/28 21:52:15, James Hawkins wrote:
> 80 cols

Done.

[James Hawkins, 2011-08-01 18:28:49]

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:71: name: undefined,
Is this intended to be private?

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:91: updateBounds:
function() {
Document this method.

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:114: /**
Add blank line between methods.

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:119: if(x.parent.pid ==
y.parent.pid) {
You could save a lot of indentation by reversing this logic and returning early
(from the else block below).

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:119: if(x.parent.pid ==
y.parent.pid) {
Space after if.

[nduca, 2011-08-01 20:52:22]

Done, thanks!

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:71: name: undefined,
On 2011/08/01 18:28:49, James Hawkins wrote:
> Is this intended to be private?

Nope, its public. Should I be exposing this via a getter?

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:91: updateBounds:
function() {
On 2011/08/01 18:28:49, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:114: /**
On 2011/08/01 18:28:49, James Hawkins wrote:
> Add blank line between methods.

Done.

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:119: if(x.parent.pid ==
y.parent.pid) {
On 2011/08/01 18:28:49, James Hawkins wrote:
> You could save a lot of indentation by reversing this logic and returning
early
> (from the else block below).

Done.

[James Hawkins, 2011-08-01 21:01:24]

LGTM

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
File chrome/browser/resources/gpu_internals/timeline_model.js (right):

http://codereview.chromium.org/7495036/diff/13001/chrome/browser/resources/gp...
chrome/browser/resources/gpu_internals/timeline_model.js:71: name: undefined,
On 2011/08/01 20:52:22, nduca wrote:
> On 2011/08/01 18:28:49, James Hawkins wrote:
> > Is this intended to be private?
> 
> Nope, its public. Should I be exposing this via a getter?

Hmm, likely though I won't ask you to do that. It's somewhat difficult to get
protected/private access right in JS.  I'd just leave it as is.

[commit-bot: I haz the power, 2011-08-03 05:07:21]

Can't apply patch for file
chrome/browser/resources/gpu_internals/timeline_model.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/gpu_internals/timeline_model.js
Hunk #6 FAILED at 233.
Hunk #7 FAILED at 274.
Hunk #8 succeeded at 334 (offset 21 lines).
Hunk #9 succeeded at 357 (offset 21 lines).
2 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/resources/gpu_internals/timeline_model.js.rej