[webkit-reviews] review denied: [Bug 101910] Web Inspector: line number of script should be search-able in Timeline Panel : [Attachment 174361] 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 174361: Patch
https://bugs.webkit.org/attachment.cgi?id=174361&action=review

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


> Source/WebCore/inspector/front-end/TimelinePanel.js:1344
> +    toSearchText += record.detailsNode() ? (" " +
record.detailsNode().textContent) : "";

Nit: you don't need ternary operator when second part is noop.
if (record.detailsNode())
    toSearchText += record.detailsNode();

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:925
> +	       this._details = this._getRecordDetails() ?
this._getRecordDetails().textContent : this._getRecordDetails();

This way you build record details node twice. I don't think you need this
method at all btw.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:959
> +	   case WebInspector.TimelineModel.RecordType.GCEvent:

Please separate formatting changes from the semantics ones. I can't track what
has changed here. I wouldn't change it at this point since we'll lose blame
capabilities.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1020
> +	   if (typeof details !== "object")

typeof details === "string"