Tons of timeline tweaks

Tons of timeline tweaks

- Dont resize or invalidate until attached to the document.
- Unregister handlers on global objects when detached.
- Dont handle input events when TimelineView is focusable.
- Reduce console spam.
- Move dragBox offscreen, fixing bug where scroll resets when dragging.
- Size heading dynamically using measuring stick
- Disable text eliding [was causing huge GC pauses]

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

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

LGTM with a nit or two

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

http://codereview.chromium.org/8359025/diff/2001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:151:
[].forEach.call(document.querySelectorAll('link[rel=stylesheet]'),
What's this do?

http://codereview.chromium.org/8359025/diff/2001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:305: maxHeadingWidth =
(maxHeadingWidth + 4) + 'px';
nit: Is this normal practice to turn a num variable into a string? I stumbled on
this line until I realized it would evaluate to a string and then convert
maxHeadingWidth to a string.

http://codereview.chromium.org/8359025/diff/2001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:537: 'Dbl-click to zoom in; Shift
dbl-click to zoom out\n';
We really should add mouse scroll zoom.

[nduca, 2011-10-26 09:59:44]

Cleaned up, thanks ;)

> chrome/browser/resources/tracing/timeline.js:537: 'Dbl-click to zoom in; Shift
> dbl-click to zoom out\n';
> We really should add mouse scroll zoom.

Yeah, though since we're doing compositor thread, what I really should do is a
20% project on out how to get a UI like this without preventing compositing
smooth scrolling. Just binding a scroll listener would force us to slow scroll.
Its not trivial... :(


@arv: your turn. :)

[nduca, 2011-10-31 17:29:09]

@arv: ping

[nduca, 2011-11-04 17:46:26]

On 2011/10/31 17:29:09, nduca wrote:
> @arv: ping

+jhawkins

[James Hawkins, 2011-11-05 17:51:37]

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:8: position: fixed;
Alphabetize properties, here and elsewhere.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:10: border: 1px solid rgb(0, 0,
96);
Use hex instead of rgb here.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:16: -webkit-box-orient: vertical;
-webkit properties should be at the top of the list.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:18: padding-bottom: 1px;
padding: 1px 0; ?

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:22: border-top: 1px solid #D0D0D0;
Hex characters should be lowercase.

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

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:143: function MeasuringStick()
Move this into a separate file.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:144: {
Opening brace should be on the same line as the method.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:168: convertMeasureToPixels_:
function(str) {
No need for this method.

parseInt(str, 10);

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:253: safeAddEventListener:
function(object, event, func, target) {
Document this method and the params.

In the comments, describe why using this method is necessary.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:253: safeAddEventListener:
function(object, event, func, target) {
This method should likely be private.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:285: // Create tracks and measure
their heading size
nit: Period at end of sentence.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:301: // limit heading width to
300px
Comment sentence should start with a capital letter; add period.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:309: // Attach tracks and set width
Add period.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:332: if
(this.isAttachedToDocument_)
Braces required for multi-line blocks.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:339: get isAttachedToDocument_() {
Document return type.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:695: }
Optional: It's generally safer to leave the trailing comma on the last entry in
the object; if the next dev to code in here doesn't remember to add it, it's a
non-obvious bug to fix.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_track.js (right):

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_track.js:96: get heading() {
Document return type.

[nduca, 2011-11-09 22:52:19]

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:8: position: fixed;
On 2011/11/05 17:51:37, James Hawkins wrote:
> Alphabetize properties, here and elsewhere.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:10: border: 1px solid rgb(0, 0,
96);
On 2011/11/05 17:51:37, James Hawkins wrote:
> Use hex instead of rgb here.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:16: -webkit-box-orient: vertical;
On 2011/11/05 17:51:37, James Hawkins wrote:
> -webkit properties should be at the top of the list.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:18: padding-bottom: 1px;
On 2011/11/05 17:51:37, James Hawkins wrote:
> padding: 1px 0; ?

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.css:22: border-top: 1px solid #D0D0D0;
On 2011/11/05 17:51:37, James Hawkins wrote:
> Hex characters should be lowercase.

Done.

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

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:143: function MeasuringStick()
On 2011/11/05 17:51:37, James Hawkins wrote:
> Move this into a separate file.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:144: {
On 2011/11/05 17:51:37, James Hawkins wrote:
> Opening brace should be on the same line as the method.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:168: convertMeasureToPixels_:
function(str) {
On 2011/11/05 17:51:37, James Hawkins wrote:
> No need for this method.
> 
> parseInt(str, 10);

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:253: safeAddEventListener:
function(object, event, func, target) {
On 2011/11/05 17:51:37, James Hawkins wrote:
> This method should likely be private.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:253: safeAddEventListener:
function(object, event, func, target) {
On 2011/11/05 17:51:37, James Hawkins wrote:
> Document this method and the params.
> 
> In the comments, describe why using this method is necessary.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:285: // Create tracks and measure
their heading size
On 2011/11/05 17:51:37, James Hawkins wrote:
> nit: Period at end of sentence.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:301: // limit heading width to
300px
On 2011/11/05 17:51:37, James Hawkins wrote:
> Comment sentence should start with a capital letter; add period.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:309: // Attach tracks and set width
On 2011/11/05 17:51:37, James Hawkins wrote:
> Add period.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:332: if
(this.isAttachedToDocument_)
On 2011/11/05 17:51:37, James Hawkins wrote:
> Braces required for multi-line blocks.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:339: get isAttachedToDocument_() {
On 2011/11/05 17:51:37, James Hawkins wrote:
> Document return type.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline.js:695: }
On 2011/11/05 17:51:37, James Hawkins wrote:
> Optional: It's generally safer to leave the trailing comma on the last entry
in
> the object; if the next dev to code in here doesn't remember to add it, it's a
> non-obvious bug to fix.

Done.

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_track.js (right):

http://codereview.chromium.org/8359025/diff/7001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_track.js:96: get heading() {
On 2011/11/05 17:51:37, James Hawkins wrote:
> Document return type.

Done.

[James Hawkins, 2011-11-10 01:06:13]

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/measuring_stick.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:6: 
nit: Remove blank line.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:22: for (var i = 0; i <
stylesheets.length; ++i) {
nit: JS style is to use i++.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:38: measure:
function(element)
nit: Opening brace goes on this line.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.css:31: padding-right: 5px;
nit: padding-right goes after padding.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:201: * Wraps the stanard
addEventListener but automatically binds the provided
s/stanard/standard/

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:259: maxHeadingWidth +=
maxHeadingWidth + 4;
What is the 4 about?

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:270: if
(this.isAttachedToDocument_) {
nit: No braces for single-line blocks.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:487: if
(this.parentElement.parentElement.tabIndex)
nit: Multi-line blocks must have braces.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:506: for (i = 0; i <
this.selection_.length; ++i) {
nit: No braces for single-line blocks.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:514: for (i = 0; i <
this.selection_.length; ++i) {
nit: No braces for single-line blocks.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline_model.js:286:
getOrCreateThread(event.tid);
4 space indentation when wrapping lines.

[nduca, 2011-11-10 02:24:35]

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/measuring_stick.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:6: 
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: Remove blank line.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:22: for (var i = 0; i <
stylesheets.length; ++i) {
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: JS style is to use i++.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/measuring_stick.js:38: measure:
function(element)
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: Opening brace goes on this line.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.css (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.css:31: padding-right: 5px;
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: padding-right goes after padding.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:201: * Wraps the stanard
addEventListener but automatically binds the provided
On 2011/11/10 01:06:13, James Hawkins wrote:
> s/stanard/standard/

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:259: maxHeadingWidth +=
maxHeadingWidth + 4;
On 2011/11/10 01:06:13, James Hawkins wrote:
> What is the 4 about?
Its some extra padding to make things look less crushed to the edge of the
heading. Should I be css'ing that?

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:270: if
(this.isAttachedToDocument_) {
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:487: if
(this.parentElement.parentElement.tabIndex)
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: Multi-line blocks must have braces.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:506: for (i = 0; i <
this.selection_.length; ++i) {
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:514: for (i = 0; i <
this.selection_.length; ++i) {
On 2011/11/10 01:06:13, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8359025/diff/15001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline_model.js:286:
getOrCreateThread(event.tid);
On 2011/11/10 01:06:13, James Hawkins wrote:
> 4 space indentation when wrapping lines.

Done.

[James Hawkins, 2011-11-11 23:37:40]

LGTM with nits.

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:216: for (var i = 0; i <
this.boundListeners_.length; ++i) {
nit: i++

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:507: for (i = 0; i <
this.selection_.length; ++i)
nit: i++

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:513: for (i = 0; i <
this.selection_.length; ++i)
nit: i++

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline_model.js:286:
getOrCreateThread(event.tid);
nit: Indentation for wrapped lines is 4 spaces from start of previous line.

In fact, if you wrap the line after '=', this would be much cleaner. Here and
line 300.

[nduca, 2011-11-12 01:28:27]

Thanks

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline.js (right):

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:216: for (var i = 0; i <
this.boundListeners_.length; ++i) {
On 2011/11/11 23:37:40, James Hawkins wrote:
> nit: i++

Done.

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:507: for (i = 0; i <
this.selection_.length; ++i)
On 2011/11/11 23:37:40, James Hawkins wrote:
> nit: i++

Done.

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline.js:513: for (i = 0; i <
this.selection_.length; ++i)
On 2011/11/11 23:37:40, James Hawkins wrote:
> nit: i++

Done.

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8359025/diff/21001/chrome/browser/resources/tr...
chrome/browser/resources/tracing/timeline_model.js:286:
getOrCreateThread(event.tid);
On 2011/11/11 23:37:40, James Hawkins wrote:
> nit: Indentation for wrapped lines is 4 spaces from start of previous line.
> 
> In fact, if you wrap the line after '=', this would be much cleaner. Here and
> line 300.

Done.