[webkit-reviews] review denied: [Bug 33995] Web Inspector: Additional instrumentation for Timeline : [Attachment 47208] Second iteration for the first patch to WebCore

[bugzilla-daemon at webkit.org]

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 47208: Second iteration for the first patch to WebCore
https://bugs.webkit.org/attachment.cgi?id=47208&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +
> +void InspectorTimelineAgent::willCallFunction(const String& name, int line)
> +{

I'd use sourceName and sourceLine as parameter names. Otherwise it sounds like
you are passing a function name.

> +   
pushCurrentRecord(TimelineRecordFactory::createFunctionCallData(m_frontend,
name, line),
> +	   FunctionCallTimelineRecordType);

Long lines are Ok in WebKit.

> +	   static int count() { return s_instanceCount; }

instanceCount()

> +	   typedef std::pair<String, int> PositionInScript;

PositionInScript might be not the best name for it (given that it contains the
name of the script as well). SourceLocation / SourceLine / ScriptNameAndLine ?

> +
> +	   Vector< PositionInScript > m_callInfoStack;
> +
>	   Vector< TimelineRecordEntry > m_recordStack;
> +
> +	   static int s_instanceCount;
>      };
>  

Nit: why so many blank lines?

> -	   element.positionAt(newElementPosition.x, newElementPosition.y);
> +	   element.positionAt(newElementPosition.x - this._offsetX,
newElementPosition.y - this._offsetY);

This does not break conditional breakpoints, right?

> +WebInspector.TimelinePanel.linkToFile = function(file, line, panel)
> +{
> +    var link = document.createElement("a");
> +    link.href = file;
> +    link.lineNumber = line;
> +    link.innerText = file + ":" +line;
> +    if (panel)
> +	   link.preferredPanel = panel;
> +    return link;
> +}
> +

There is a _formaterror function in ConsoleView.js that uses specific style for
the link and WebInspector.displayNameForURL to render the source name nicely. I
think it is worth extracting that code into a
WebInspector.createLinkToSourceLine function and using it here.
(WebInspector.createLineToSourceLine should be defined in inspector.js in that
case).

> -		   sendRequestRecord.details = this._getRecordDetails(record);
> +		   record.details = this._getRecordDetails(sendRequestRecord);
> +		   record.callerScriptName =
sendRequestRecord.callerScriptName;
> +		   record.callerScriptLine =
sendRequestRecord.callerScriptLine;

This is wrong. We receive 'record' from the agent. But that is not something we
render. We create a formattedRecrod object that is basically a visual
representation of the record we'd like to render. formattedRecord references
record via its property. record.details has no meaning, no need to assign
anything to it.

I am aslo confused with the "sendRequestRecord.details =
this._getRecordDetails(record)." Not sure why it is needed. My only guess is
that corresponding record entry did not url info before and I needed to steal
if from receivedResponse one.

Another thing is that you should not provide response received / resource
finished records with the callers. Resource events are asynchronous, not
belonging to the js execution flow. Send request record is probably synchronous
and should have it. So everything is all right without the
"record.callerScriptName = changes" parts. Caller info is missing here for a
reason!

>	       }
>	   } else if (record.type ===
WebInspector.TimelineAgent.RecordType.ResourceFinish) {
>	       var sendRequestRecord =
this._sendRequestRecords[record.data.identifier];
> -	       if (sendRequestRecord) // False for main resource.
> +	       if (sendRequestRecord) {// False for main resource.
>		   formattedRecord.startTime =
sendRequestRecord._responseReceivedFormattedTime;
> +		   record.callerScriptName =
sendRequestRecord.callerScriptName;
> +		   record.callerScriptLine =
sendRequestRecord.callerScriptLine;
> +	       }

Ditto.

> +	   } else if (record.type ===
WebInspector.TimelineAgent.RecordType.TimerInstall)
> +	       this._timerRecords[record.data.timerId] = formattedRecord;
> +	   else if (record.type ===
WebInspector.TimelineAgent.RecordType.TimerFire) {
> +	       var timerInstalledRecord =
this._timerRecords[record.data.timerId];
> +	       if (timerInstalledRecord) {
> +		   record.callerScriptName =
timerInstalledRecord.callerScriptName;
> +		   record.callerScriptLine =
timerInstalledRecord.callerScriptLine;

Also wrong - timer fire is initiated by the host timer. So caller should be
missing here.

> +	       }
> +	   } else if (record.type ===
WebInspector.TimelineAgent.RecordType.TimerRemove) {
> +	       var timerInstalledRecord =
this._timerRecords[record.data.timerId];
> +	       if (timerInstalledRecord) {
> +		   record.callerScriptName =
timerInstalledRecord.callerScriptName;
> +		   record.callerScriptLine =
timerInstalledRecord.callerScriptLine;

This is wrong for another reason. Either timer was removed by user (so it
should already have proper caller info). Or by some other reason, but not from
the install record's caller.


> +	   case WebInspector.TimelineAgent.RecordType.FunctionCall:
> +	       return record.data.scriptName + ":" + record.data.scriptLine;

See hint on formatting urls above.