[webkit-reviews] review canceled: [Bug 33995] Web Inspector: Additional instrumentation for Timeline : [Attachment 49575] The same patch but fixed inspector's layout tests

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled 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 49575: The same patch but fixed inspector's layout tests
https://bugs.webkit.org/attachment.cgi?id=49575&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
th * 2 };
> +	   var newElementPosition = { x: 0, y: 0, width: preferredWidth +
scrollerWidth, height: preferredHeight };
> +	   newElementPosition.heightWithArrow = newElementPosition.height +
arrowHeight;

Why not to assign inline?

> +	       if (newElementPosition.y + newElementPosition.heightWithArrow -
borderWidth >= totalHeight) {
> +		   newElementPosition.height = totalHeight - anchorBox.y -
anchorBox.height - borderRadius * 2 - arrowHeight;
>	       }

No need for { }

> +	       if (sendRequestRecord) {// False for main resource.
>		   formattedRecord.startTime =
sendRequestRecord._responseReceivedFormattedTime;
> +		   formattedRecord.callerScriptName =
sendRequestRecord.callerScriptName;
> +		   formattedRecord.callerScriptLine =
sendRequestRecord.callerScriptLine;

We wanted to remove this, right?

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

I am not a great fan of this code, would prefer link to the timer install
instead.