[webkit-reviews] review denied: [Bug 33995] Web Inspector: Additional instrumentation for Timeline : [Attachment 47195] Additional Timeline instrumentation.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 22 06:28:28 PST 2010
Pavel Feldman <pfeldman at chromium.org> has denied review:
Bug 33995: Web Inspector: Additional instrumentation for Timeline
https://bugs.webkit.org/show_bug.cgi?id=33995
Attachment 47195: Additional Timeline instrumentation.
https://bugs.webkit.org/attachment.cgi?id=47195&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> + There are 3 patches should be applied:
> + The first one is a patch for WebInspector with popup dialog and the
changes for
> + Caller info support for all kind of Timeline events and new Function
Call event.
> + The second one will be the patch in V8 code for Function Call info.
> + The third one will be in V8 bindings for pushing Caller info to
Timeline.
> +
What about JSC?
>
> +int InspectorTimelineAgent::m_count = 0;
> +
s_instanceCount?
> + copyCallerInfoToRecord(record);
populateCallerInfo?
> + static ScriptObject createCallFunctionData(InspectorFrontend*, const
String& name, int lineNumber);
> +
createFunctionCallData?
> -button.status-bar-item.toggled-1 .glyph {
> - background-color: rgb(66, 129, 235);
> -}
> -
> -button.status-bar-item.toggled-2 .glyph {
> - background-color: purple;
> -}
> -
Please rebase.
>
> +function _linkToFile(file, line, panel)
> +{
Either put this into utilities.js or declare on TimelinePanel's constructor.
> + this._lastRecord.callFromName ==
formattedRecord.callFromName &&
> + this._lastRecord.callFromLine ==
formattedRecord.callFromLine &&
callerURL, callerLine?
> + this._recordStyles[recordTypes.FunctionCall] = { title:
WebInspector.UIString("Function Call"), category: this.categories.scripting };
Ad this to WebCore/English.lproj/localizedStrings.js
> + } else if (record.type ===
WebInspector.TimelineAgent.RecordType.TimerInstall) {
> + this._timerRecords[record.data.timerId] = formattedRecord;
> + } else if (record.type ===
WebInspector.TimelineAgent.RecordType.TimerFire) {
No need for {}
> + case WebInspector.TimelineAgent.RecordType.FunctionCall:
> + return record.data.scriptName + ":" + (1 +
record.data.scriptLine);
Mind JSC vs V8 difference, add in bindings instead.
> + this._callInfo = document.createElement("span");
> + this._callInfo.className = "data dimmed";
> + this._callInfo.textContent = "";
> +
I don't think we need this in the list.
> + _createCell: function(content)
> + {
> + var text = document.createElement("label");
> + text.className = "popup-message";
> + text.appendChild(document.createTextNode(content));
> + var cell = document.createElement("td");
> + cell.appendChild(text);
> + return cell;
> + },
> +
Do not use table / td if possible.
> + typedef std::pair<String, int> PositionInScript;
> +
private?
> + void pushCallerInfo(const String& scriptName, int scriptLine) {
> +
m_callInfoStack.append(InspectorTimelineAgent::PositionInScript(scriptName,
scriptLine));
> + }
> +
> + void popCallerInfo() {
> + ASSERT(m_callInfoStack.size() != 1);
> + m_callInfoStack.shrink(m_callInfoStack.size() - 1);
> + }
> +
We do not generally put code into headers.
> private:
> + void copyCallerInfoToRecord(ScriptObject& record) {
> + PositionInScript& callInfo = m_callInfoStack.last();
> + record.set("callFromName", callInfo.first);
> + record.set("callFromLine", callInfo.second);
> + }
> +
Move to cpp
More information about the webkit-reviews
mailing list