[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