[webkit-reviews] review denied: [Bug 33995] Web Inspector: Additional instrumentation for Timeline : [Attachment 48902] Next version of Timeline improvenents

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> 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 48902: Next version of Timeline improvenents
https://bugs.webkit.org/attachment.cgi?id=48902&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +	   static bool getCallerScriptInfo(String& sourceName, int&
sourceLineNumber);
> +

callScriptInfo (or even better callLocation)
return parameters should be passed by pointer.

>  {
>      ScriptObject record =
TimelineRecordFactory::createGenericRecord(m_frontend,
currentTimeInMilliseconds());
>      record.set("data",
TimelineRecordFactory::createGenericTimerData(m_frontend, timerId));
> +    populateCallerInfo(record);
>      addRecordToTimeline(record, TimerRemoveTimelineRecordType);
>  }

We can inline populateCallerInfo into the createGenericRecord.

>  } // namespace WebCore

Warning, strange indent above this line.

>  
>	   // Make resource receive record last since request was sent; make
finish record last since response received.
>	   if (record.type ===
WebInspector.TimelineAgent.RecordType.ResourceSendRequest) {
> @@ -210,12 +217,32 @@ WebInspector.TimelinePanel.prototype = {
>	       if (sendRequestRecord) { // False if we started instrumentation
in the middle of request.
>		   sendRequestRecord._responseReceivedFormattedTime =
formattedRecord.startTime;
>		   formattedRecord.startTime = sendRequestRecord.startTime;
> -		   sendRequestRecord.details = this._getRecordDetails(record);
> +		   formattedRecord.details =
this._getRecordDetails(sendRequestRecord);
> +		   formattedRecord.callerScriptName =
sendRequestRecord.callerScriptName;
> +		   formattedRecord.callerScriptLine =
sendRequestRecord.callerScriptLine;

No need to set javascript caller to the event that is dispatched from other
places (timer, network, etc.).


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

Timer remove has its own caller info.
> +	   if (document._bubble) {
> +	       document._bubble.hide();
> +	   }

Non need for { }

Let us look at bubble together and we will merge it with Popover.