WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95445
Web Inspector: Implement search and filtering on Timeline panel
https://bugs.webkit.org/show_bug.cgi?id=95445
Summary
Web Inspector: Implement search and filtering on Timeline panel
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
Details
Formatted Diff
Diff
Patch
(14.07 KB, patch)
2012-08-30 08:11 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Patch
(14.64 KB, patch)
2012-08-31 08:07 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Patch
(14.65 KB, patch)
2012-09-03 02:54 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Patch
(15.08 KB, patch)
2012-09-07 03:01 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Patch
(15.06 KB, patch)
2012-09-07 06:36 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Patch
(15.11 KB, patch)
2012-09-12 02:11 PDT
,
eustas.bug
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
eustas.bug
Comment 1
2012-08-30 05:42:07 PDT
Created
attachment 161453
[details]
Patch
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
Created
attachment 161472
[details]
Patch
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
Created
attachment 161703
[details]
Patch
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
Created
attachment 161887
[details]
Patch
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
Created
attachment 162722
[details]
Patch
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
Created
attachment 162756
[details]
Patch
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
Created
attachment 163553
[details]
Patch
Andrey Kosyakov
Comment 22
2012-09-12 02:33:31 PDT
Committed
r128281
: <
http://trac.webkit.org/changeset/128281
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug