[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