[webkit-reviews] review denied: [Bug 101910] Web Inspector: line number of script should be search-able in Timeline Panel : [Attachment 174122] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied pdeng6 <pan.deng at intel.com>'s
request for review:
Bug 101910: Web Inspector: line number of script should be search-able in
Timeline Panel
https://bugs.webkit.org/show_bug.cgi?id=101910

Attachment 174122: Patch
https://bugs.webkit.org/attachment.cgi?id=174122&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174122&action=review


> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:888
> +		      
contentHelper._appendTextRow(WebInspector.UIString("Details"),
this.detailsNode().textContent);

I wonder if you should do appendElementRow here in order to not lose the
formatting and save on operations.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:930
> +		   detailsContainer.appendChild(document.createTextNode("("));

I'd bring this back - otherwise, you can't reuse it in the popup above and you
add "(" twice in the search matcher.

I would simply replace 
return this.data ? this.data["type"] : null;
with
return this.data ? createSpan(this.data["type"]) : null;
in _getRecordDetails

I think it is Ok to have it nullable Node. I.e. 
/**
 * @return {?Node}
 */
_getRecordDetails

caching could be done via testing for (typeof this._details === "undefined") so
that you don't compute null over and over.