[webkit-reviews] review denied: [Bug 95445] Web Inspector: Implement search and filtering on Timeline panel : [Attachment 161472] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied eustas.bug at gmail.com's request
for review:
Bug 95445: Web Inspector: Implement search and filtering on Timeline panel
https://bugs.webkit.org/show_bug.cgi?id=95445

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

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


> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:1232
> +WebInspector.TimelineWindowFilter = function(pane)

Why did this change?

> Source/WebCore/inspector/front-end/TimelinePanel.js:149
> +    this._clearSearchResults();

No need to clear things - we don't have results yet.

> Source/WebCore/inspector/front-end/TimelinePanel.js:150
> +    this._searchFilter = new WebInspector.TimelineSearchFilter(this);

We should create things lazily.

> Source/WebCore/inspector/front-end/TimelinePanel.js:753
> +	       this._scheduleRefresh(true);

Do you want to refresh instantly?

> Source/WebCore/inspector/front-end/TimelinePanel.js:1018
> +	   var index = this._searchResults.indexOf(this._selectedSearchResult)
+ 1;

Why not to store this._selectedSearchResultIndex instead to make things faster
and simpler ?

> Source/WebCore/inspector/front-end/TimelinePanel.js:1035
> +	   WebInspector.searchController.updateCurrentMatchIndex(index, this);

You want to reveal it here?

> Source/WebCore/inspector/front-end/TimelinePanel.js:1048
> +	   for (var element = this._sidebarListElement.firstChild; element;
element = element.nextSibling) {

This woun't work: _sidebarListElement is a viewport, it does not have all
elements in it.

> Source/WebCore/inspector/front-end/TimelinePanel.js:1065
> +    _updateSearchResults: function(revealRecord)

Please annotate the revealRecord parameter.

> Source/WebCore/inspector/front-end/TimelinePanel.js:1293
> +	       WebInspector.highlightSearchResult(this._typeElement,
matchInfo.index, matchInfo[0].length, domChanges);

You want a regex that can span across type and data ("Paint 800")

> Source/WebCore/inspector/front-end/TimelinePanel.js:1468
> +	   return this._panel._searchRegExp.test(record.title) ||
this._panel._searchRegExp.test(record.details());

What do we do when TimelineRecordListRow changes? You should move this method
into TimelineRecordListRow (could be static).