[webkit-reviews] review denied: [Bug 33995] Web Inspector: Additional instrumentation for Timeline : [Attachment 49168] Next iteration with new Popover balloons. Can be landed after v8 deps roll.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 22 03:01:28 PST 2010


Timothy Hatcher <timothy at hatcher.name> has denied Ilya Tikhonovsky
<loislo at google.com>'s request for review:
Bug 33995: Web Inspector: Additional instrumentation for Timeline
https://bugs.webkit.org/show_bug.cgi?id=33995

Attachment 49168: Next iteration with new Popover balloons. Can be landed after
v8 deps roll.
https://bugs.webkit.org/attachment.cgi?id=49168&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
I'm not awake enough for a detailed line by line review but here are soome
highlevel comments and questions:

Why is there a thicker padding at the bottom of the popover in the screenshot?

The labels should be consistent and Title Case. But I think "Single shot"
should be renamed to be "Repeats" and flip the boolean. And "installed at"
should be "Call Site" or something not ending in "at".

hideAtClick should be named "hideWhenClicked" and would be more logical as a
setter, but a function is fine if only just renamed.

You have some style problems in at least TimelinePanel.js:

 640		 }
 641		 else if ( this._record.type ==
WebInspector.TimelineAgent.RecordType.FunctionCall ) {
 642		     var link =
WebInspector.linkifyResourceAsNode(this._record.data.scriptName, "scripts",
this._record.data.scriptLine);
 643		    
bubbleContentTable.appendChild(this._createLinkRow(WebInspector.UIString("Locat
ion"), link));
 644		 }
 645		 else
 646		    
bubbleContentTable.appendChild(this._createRow(WebInspector.UIString("Details")
,


More information about the webkit-reviews mailing list