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

Description eustas.bug 2012-08-30 04:48:13 PDT
Implement textual search/filtering on Timeline panel.
Comment 1 eustas.bug 2012-08-30 05:42:07 PDT
Created attachment 161453 [details]
Patch
Comment 2 Peter Beverloo (cr-android ews) 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
Comment 3 Andrey Kosyakov 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?
Comment 4 eustas.bug 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
Comment 5 eustas.bug 2012-08-30 08:11:44 PDT
Created attachment 161472 [details]
Patch
Comment 6 Andrey Kosyakov 2012-08-31 02:53:31 PDT
Comment on attachment 161472 [details]
Patch

LGTM
Comment 7 Pavel Feldman 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).
Comment 8 eustas.bug 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.
Comment 9 eustas.bug 2012-08-31 08:07:36 PDT
Created attachment 161703 [details]
Patch
Comment 10 Andrey Kosyakov 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?
Comment 11 eustas.bug 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.
Comment 12 eustas.bug 2012-09-03 02:54:10 PDT
Created attachment 161887 [details]
Patch
Comment 13 Andrey Kosyakov 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?
Comment 14 eustas.bug 2012-09-07 03:01:28 PDT
Created attachment 162722 [details]
Patch
Comment 15 eustas.bug 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)
Comment 16 Yury Semikhatsky 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?
Comment 17 eustas.bug 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.
Comment 18 eustas.bug 2012-09-07 06:36:50 PDT
Created attachment 162756 [details]
Patch
Comment 19 Yury Semikhatsky 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.
Comment 20 eustas.bug 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.
Comment 21 eustas.bug 2012-09-12 02:11:00 PDT
Created attachment 163553 [details]
Patch
Comment 22 Andrey Kosyakov 2012-09-12 02:33:31 PDT
Committed r128281: <http://trac.webkit.org/changeset/128281>