Web Inspector: Implement search and filtering on Timeline panel

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

Implement textual search/filtering on Timeline panel.

Created attachment 161453 [details]
Patch

[Peter Beverloo (cr-android ews)]

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 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 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

Created attachment 161472 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 161472 [details]
Patch

LGTM

[Pavel Feldman]

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 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.

Created attachment 161703 [details]
Patch

[Andrey Kosyakov]

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 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.

Created attachment 161887 [details]
Patch

[Andrey Kosyakov]

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?

Created attachment 162722 [details]
Patch

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 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 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.

Created attachment 162756 [details]
Patch

[Yury Semikhatsky]

(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.

> > Actually, the caller (_updateSearchHighlight) makes that check for us.
> You can move the check inside the method then.
Fixed.

Created attachment 163553 [details]
Patch

[Andrey Kosyakov]

Committed r128281: <http://trac.webkit.org/changeset/128281>