In timeline panel, line number of script is not search-able, such as item: Function Call(codesite_head.pack.04102009.js:75) 04102009.js is search-able, however, it is not search-able if with line number "75", it makes folks confused I think.
Created attachment 173588 [details] the expected result of searching a line number of js.
Created attachment 173598 [details] Patch
Comment on attachment 173598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173598&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:1361 > + if (details && details.hasOwnProperty('textContent')) Would be nice if we could hide this logic in record and have something like record.textForSearch()
Comment on attachment 173598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173598&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:1361 >> + if (details && details.hasOwnProperty('textContent')) > > Would be nice if we could hide this logic in record and have something like record.textForSearch() WebInspector.Linkifier.prototype._updateAnchor and WebInspector.linkifyURLAsNode should be changed to supply resulting anchor with corresponding expando attribute.
Created attachment 173890 [details] Patch
(In reply to comment #3) > (From update of attachment 173598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173598&action=review > > > Source/WebCore/inspector/front-end/TimelinePanel.js:1361 > > + if (details && details.hasOwnProperty('textContent')) > > Would be nice if we could hide this logic in record and have something like record.textForSearch() done, thanks:)
(In reply to comment #4) > (From update of attachment 173598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173598&action=review > > >> Source/WebCore/inspector/front-end/TimelinePanel.js:1361 > >> + if (details && details.hasOwnProperty('textContent')) > > > > Would be nice if we could hide this logic in record and have something like record.textForSearch() > > WebInspector.Linkifier.prototype._updateAnchor and WebInspector.linkifyURLAsNode should be changed to supply resulting anchor with corresponding expando attribute. Could you please describe the reason? thanks.
Comment on attachment 173890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173890&action=review > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:930 > + if (details && details.hasOwnProperty('textContent')) You should check for (details instanceof Node) and then use textContent. But as Eugene mentioned, the fact that details() returns either string or Node is the root of the problem. I'd rename details() to detailsNode() and create a span with text in case of text content there. Then I would use detailsNode.textContent in testContentMatching and removed the similar type check from WebInspector.TimelineRecordListRow.prototype.update + would also use appendElementRow in _generatePopupContentWithImagePreview.
Created attachment 174122 [details] Patch
(In reply to comment #8) > (From update of attachment 173890 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173890&action=review > > > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:930 > > + if (details && details.hasOwnProperty('textContent')) > > You should check for (details instanceof Node) and then use textContent. > > But as Eugene mentioned, the fact that details() returns either string or Node is the root of the problem. I'd rename details() to detailsNode() and create a span with text in case of text content there. Then I would use detailsNode.textContent in testContentMatching and removed the similar type check from WebInspector.TimelineRecordListRow.prototype.update + would also use appendElementRow in _generatePopupContentWithImagePreview. Good point, thanks, Done!
Comment on attachment 174122 [details] Patch 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.
Created attachment 174361 [details] Patch
(In reply to comment #11) > (From update of attachment 174122 [details]) > 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. agree, TimelineRecordListRow.prototype.update and testContentMatching need "()", however popup won't use it, so I write detailsNode() & details() > > I would simply replace > return this.data ? this.data["type"] : null; > with > return this.data ? createSpan(this.data["type"]) : null; > in _getRecordDetails Good point, original _getRecordDetails may return {null|undefined|Node|string} now, changed to {null|Node} > > 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. Good idea, done!
Comment on attachment 174361 [details] Patch 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"
Created attachment 174369 [details] Patch
(In reply to comment #14) > (From update of attachment 174361 [details]) > 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(); > thanks, done > > 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. agree, removed this method. > > > 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. see :), formats will be changed in another patch. > > > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1020 > > + if (typeof details !== "object") > > typeof details === "string" done, thanks.
Comment on attachment 174369 [details] Patch Attachment 174369 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14845397 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 174369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174369&action=review > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:888 > + contentHelper._appendTextRow(WebInspector.UIString("Details"), this.detailsNode().childNodes[1].textContent); So this is a third time I am suggesting to call appendElementRow here :) Is there a reason why this is bad? > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1006 > + if (!details) You don't need this check now.
(In reply to comment #18) > (From update of attachment 174369 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174369&action=review > > > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:888 > > + contentHelper._appendTextRow(WebInspector.UIString("Details"), this.detailsNode().childNodes[1].textContent); > > So this is a third time I am suggesting to call appendElementRow here :) Is there a reason why this is bad? I've tested it, appendElementRow will move the node from TimelineRecordListRow to contentHelper, did I mis-use this function? or a deep cloned node needed? > > > Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1006 > > + if (!details) > > You don't need this check now. without this, this method may return undefined, and the testing for (typeof this._details === "undefined") will not work. I would like to change as < return details ? details : null >, how do you think of it?
Comment on attachment 174369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174369&action=review >>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:888 >>> + contentHelper._appendTextRow(WebInspector.UIString("Details"), this.detailsNode().childNodes[1].textContent); >> >> So this is a third time I am suggesting to call appendElementRow here :) Is there a reason why this is bad? > > I've tested it, appendElementRow will move the node from TimelineRecordListRow to contentHelper, did I mis-use this function? or a deep cloned node needed? Yeah, you probably want to clone it and you don't want ( and ) to be a part of the details node, right? >>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1006 >>> + if (!details) >> >> You don't need this check now. > > without this, this method may return undefined, and the testing for (typeof this._details === "undefined") will not work. > I would like to change as < return details ? details : null >, how do you think of it? Just put var details = null; above?
Created attachment 174659 [details] Patch
(In reply to comment #20) > (From update of attachment 174369 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174369&action=review > > >>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:888 > >>> + contentHelper._appendTextRow(WebInspector.UIString("Details"), this.detailsNode().childNodes[1].textContent); > >> > >> So this is a third time I am suggesting to call appendElementRow here :) Is there a reason why this is bad? > > > > I've tested it, appendElementRow will move the node from TimelineRecordListRow to contentHelper, did I mis-use this function? or a deep cloned node needed? > > Yeah, you probably want to clone it and you don't want ( and ) to be a part of the details node, right? thanks. cloned it. > > >>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1006 > >>> + if (!details) > >> > >> You don't need this check now. > > > > without this, this method may return undefined, and the testing for (typeof this._details === "undefined") will not work. > > I would like to change as < return details ? details : null >, how do you think of it? > > Just put var details = null; above? I think "var details = null" won't solve, "details" may be set undefined during switch..
Comment on attachment 174659 [details] Patch Rejecting attachment 174659 [details] from commit-queue. pan.deng@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 174659 [details] Patch Clearing flags on attachment: 174659 Committed r135254: <http://trac.webkit.org/changeset/135254>
All reviewed patches have been landed. Closing bug.