Bug 95445

Summary: Web Inspector: Implement search and filtering on Timeline panel
Product: WebKit Reporter: eustas.bug
Component: Web Inspector (Deprecated)Assignee: eustas.bug
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, dglazkov, eustas.bug, joepeck, keishi, loislo, peter+ews, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 95731    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

eustas.bug
Reported 2012-08-30 04:48:13 PDT
Implement textual search/filtering on Timeline panel.
Attachments
Patch (14.88 KB, patch)
2012-08-30 05:42 PDT, eustas.bug
no flags
Patch (14.07 KB, patch)
2012-08-30 08:11 PDT, eustas.bug
no flags
Patch (14.64 KB, patch)
2012-08-31 08:07 PDT, eustas.bug
no flags
Patch (14.65 KB, patch)
2012-09-03 02:54 PDT, eustas.bug
no flags
Patch (15.08 KB, patch)
2012-09-07 03:01 PDT, eustas.bug
no flags
Patch (15.06 KB, patch)
2012-09-07 06:36 PDT, eustas.bug
no flags
Patch (15.11 KB, patch)
2012-09-12 02:11 PDT, eustas.bug
no flags
eustas.bug
Comment 1 2012-08-30 05:42:07 PDT
Peter Beverloo (cr-android ews)
Comment 2 2012-08-30 05:51:15 PDT
Comment on attachment 161453 [details] Patch Attachment 161453 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13690670
Andrey Kosyakov
Comment 3 2012-08-30 06:10:27 PDT
Comment on attachment 161453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161453&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:724 > + function findRecordToReveal(record) { { => new line. > Source/WebCore/inspector/front-end/TimelinePanel.js:1044 > + if (this._searchFilter.enabled) { > + return; > + } drop {} > Source/WebCore/inspector/front-end/TimelinePanel.js:1048 > + if (!record) { > + return; > + } ditto > Source/WebCore/inspector/front-end/TimelinePanel.js:1051 > + do { for (var element = ... ; element; element = element.nextSibling) > Source/WebCore/inspector/front-end/TimelinePanel.js:1089 > + if (selectedIndex == -1) === > Source/WebCore/inspector/front-end/TimelinePanel.js:1486 > + if (this._matchedRecords) > + this._matchedRecords.push(record); can we avoid abusing this filter to also perform collection of the records? Apparently, it should only be a part of filter chain when we're in "filter" mode, and we could perhaps run it separately upon all records within the window when we're in "search" mode?
eustas.bug
Comment 4 2012-08-30 08:09:46 PDT
Comment on attachment 161453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161453&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:724 >> + function findRecordToReveal(record) { > > { => new line. Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1044 >> + } > > drop {} Fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1048 >> + } > > ditto Fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1051 >> + do { > > for (var element = ... ; element; element = element.nextSibling) Done >> Source/WebCore/inspector/front-end/TimelinePanel.js:1089 >> + if (selectedIndex == -1) > > === Fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1486 >> + this._matchedRecords.push(record); > > can we avoid abusing this filter to also perform collection of the records? Apparently, it should only be a part of filter chain when we're in "filter" mode, and we could perhaps run it separately upon all records within the window when we're in "search" mode? OK
eustas.bug
Comment 5 2012-08-30 08:11:44 PDT
Andrey Kosyakov
Comment 6 2012-08-31 02:53:31 PDT
Comment on attachment 161472 [details] Patch LGTM
Pavel Feldman
Comment 7 2012-08-31 05:38:18 PDT
Comment on attachment 161472 [details] Patch 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).
eustas.bug
Comment 8 2012-08-31 06:40:32 PDT
Comment on attachment 161472 [details] Patch 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? To improve readability. Method name ("accept") is somewhat ambiguous when occurs outside of filtering context. >> Source/WebCore/inspector/front-end/TimelinePanel.js:149 >> + this._clearSearchResults(); > > No need to clear things - we don't have results yet. surely. >> Source/WebCore/inspector/front-end/TimelinePanel.js:150 >> + this._searchFilter = new WebInspector.TimelineSearchFilter(this); > > We should create things lazily. OK >> Source/WebCore/inspector/front-end/TimelinePanel.js:753 >> + this._scheduleRefresh(true); > > Do you want to refresh instantly? No, because setting scrollTop also could cause (deferred) refresh. >> 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 ? I have been doing so before refactoring. Using index makes things are less simpler, by contraries. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1035 >> + WebInspector.searchController.updateCurrentMatchIndex(index, this); > > You want to reveal it here? No. Revealing record is costly. We are forcing revealing record only if it is off-screen. "_highlightSelectedSearchResult" actually checks, if selected record is visible or off-screen. >> 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. This actually works: we highlight record if it is in viewport, and reveal record if it is off-screen. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1065 >> + _updateSearchResults: function(revealRecord) > > Please annotate the revealRecord parameter. OK >> 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") We can easily do that; but is that what we really want? >> 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). OK.
eustas.bug
Comment 9 2012-08-31 08:07:36 PDT
Andrey Kosyakov
Comment 10 2012-09-03 02:33:03 PDT
Comment on attachment 161703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161703&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:1306 > +WebInspector.TimelineRecordListRow.testContentMatching = function(record, regExp) { { => next line > Source/WebCore/inspector/front-end/TimelinePanel.js:1307 > + return regExp.test(record.title) || regExp.test(record.details()); Let's have something like regExp.test(record.title + " (" + record.details + ")") instead?
eustas.bug
Comment 11 2012-09-03 02:49:34 PDT
Comment on attachment 161703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161703&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:1306 >> +WebInspector.TimelineRecordListRow.testContentMatching = function(record, regExp) { > > { => next line Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1307 >> + return regExp.test(record.title) || regExp.test(record.details()); > > Let's have something like regExp.test(record.title + " (" + record.details + ")") instead? Done.
eustas.bug
Comment 12 2012-09-03 02:54:10 PDT
Andrey Kosyakov
Comment 13 2012-09-03 09:22:07 PDT
Comment on attachment 161887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161887&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:838 > + this._updateSearchResults(false); So we would do a search upon every screen update, including scroll/resize? This looks a bit suboptimal. > Source/WebCore/inspector/front-end/TimelinePanel.js:1058 > + if (this._highlightDomChanges) nit: initialize in constructor instead?
eustas.bug
Comment 14 2012-09-07 03:01:28 PDT
eustas.bug
Comment 15 2012-09-07 03:04:01 PDT
Comment on attachment 161887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161887&action=review >> Source/WebCore/inspector/front-end/TimelinePanel.js:1058 >> + if (this._highlightDomChanges) > > nit: initialize in constructor instead? "We should create things lazily." (C) Pavel Feldman (see previous patch review)
Yury Semikhatsky
Comment 16 2012-09-07 04:59:37 PDT
Comment on attachment 162722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162722&action=review > Source/WebCore/ChangeLog:9 > + Fixed "revealRecordAt" - now it searches scans all records in window, typo: searches scans -> scans > Source/WebCore/inspector/front-end/TimelinePanel.js:1035 > + var index = this._searchResults.indexOf(this._selectedSearchResult) + this._searchResults.length + offset; We can store _selectedSearchResultIndex instead of the record itself. It would be updated every time search results change, you already do it in _updateSearchResults. That way you want need to do indexOf on each jump. > Source/WebCore/inspector/front-end/TimelinePanel.js:1036 > + this._selectSearchResult(index % this._searchResults.length); nit: please extract index = (index + this._searchResults.length) % this._searchResults.length in a separate statement. > Source/WebCore/inspector/front-end/TimelinePanel.js:1093 > + _updateSearchResults: function(searchRegExp) { Why do you need to pass searchRegExp as a parameter while it is always stored in this._searchRegExp?
eustas.bug
Comment 17 2012-09-07 06:13:51 PDT
Comment on attachment 162722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162722&action=review >> Source/WebCore/ChangeLog:9 >> + Fixed "revealRecordAt" - now it searches scans all records in window, > > typo: searches scans -> scans Fixed. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1035 >> + var index = this._searchResults.indexOf(this._selectedSearchResult) + this._searchResults.length + offset; > > We can store _selectedSearchResultIndex instead of the record itself. It would be updated every time search results change, you already do it in _updateSearchResults. That way you want need to do indexOf on each jump. I've stored index in my take-one patch. It looked clumsy. Actually, indexOf is very cheap: 17K+ searches per second for last element in 10K element array. Holding just record makes the code clean and straightforward, especially in cases of cache invalidation. >> Source/WebCore/inspector/front-end/TimelinePanel.js:1036 >> + this._selectSearchResult(index % this._searchResults.length); > > nit: please extract index = (index + this._searchResults.length) % this._searchResults.length in a separate statement. fixed >> Source/WebCore/inspector/front-end/TimelinePanel.js:1093 >> + _updateSearchResults: function(searchRegExp) { > > Why do you need to pass searchRegExp as a parameter while it is always stored in this._searchRegExp? We need regExp to be accessible for current scope - so it must be either variable or parameter. In this case using parameter makes code more clear - with type annotation we see that regExp is not null. Actually, the caller (_updateSearchHighlight) makes that check for us.
eustas.bug
Comment 18 2012-09-07 06:36:50 PDT
Yury Semikhatsky
Comment 19 2012-09-12 00:50:52 PDT
(In reply to comment #17) > (From update of attachment 162722 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162722&action=review > >> Source/WebCore/inspector/front-end/TimelinePanel.js:1093 > >> + _updateSearchResults: function(searchRegExp) { > > > > Why do you need to pass searchRegExp as a parameter while it is always stored in this._searchRegExp? > > We need regExp to be accessible for current scope - so it must be either variable or parameter. > In this case using parameter makes code more clear - with type annotation we see that regExp is not null. > Actually, the caller (_updateSearchHighlight) makes that check for us. You can move the check inside the method then.
eustas.bug
Comment 20 2012-09-12 01:10:36 PDT
> > Actually, the caller (_updateSearchHighlight) makes that check for us. > You can move the check inside the method then. Fixed.
eustas.bug
Comment 21 2012-09-12 02:11:00 PDT
Andrey Kosyakov
Comment 22 2012-09-12 02:33:31 PDT
Note You need to log in before you can comment on or make changes to this bug.