Bug 154924 – Web Inspector: hook up grid row filtering in the new Timelines UI

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Hook up grid row filtering in the new Timelines UI. Previously filtering operated on the content tree outlines maintained by the navigation sidebar panel, which has been removed. Filtering now needs to be performed on grid data.

* NOTES
This provides an opportunity to improve filtering in timeline views. Where previously only data associated with a record's tree element (usually titles/subtitles) could be queried, we can now use the cell data for the entire grid row.

[Radar WebKit Bug Importer]

<rdar://problem/24934607>

[Matt Baker]

Created attachment 276983 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 276983 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=276983&action=review

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1690
> +    NodeWasFiltered: "timelinedatagrid-node-was-filtered"

Drop timeline from the string.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:722
> +        let filters = this._filterBarNavigationItem.filterBar.filters;
> +        this.currentTimelineView.updateFilter(filters);

Nit: Could be one line.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:165
> +        });
> +        this._timelineDataGrid.addEventListener(WebInspector.DataGrid.Event.NodeWasFiltered, (event) => {

Nit: Add newline.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:172
> +        });
> +        this._timelineDataGrid.addEventListener(WebInspector.DataGrid.Event.FilterDidChange, (event) => {

Nit: Add newline.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:190
> +        if (filters)
> +            this._dataGrid.filterText = filters.text;
> +        else
> +            this._dataGrid.filterDidChange();

I suspect you would want to do: this._dataGrid.filterText = filters ? filters.text : ""; Otherwise the filterText would not clear on DataGird.

[Matt Baker]

Comment on attachment 276983 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=276983&action=review

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:352
> +        let filterableData = node.displayName();

This is specific to TimelineDataGridNode. I'll make generic in the next revision, will require additional review.

[Matt Baker]

I just noticed that this patch also breaks filtering in the Network tab.

[Timothy Hatcher]

(In reply to comment #5)
> I just noticed that this patch also breaks filtering in the Network tab.

Can you make the Network tab use the Data grid filtering?

[Matt Baker]

(In reply to comment #6)
> (In reply to comment #5)
> > I just noticed that this patch also breaks filtering in the Network tab.
> 
> Can you make the Network tab use the Data grid filtering?

I'll look into it. The data in the tree outline should still be filterable, as long as the resource URL is in the array returned by ResourceDataGridNode's override of DataGridNode's filterableData getter.

[Matt Baker]

Created attachment 277125 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 277125 [details]
[Patch] Proposed Fix

Do we need to make a change to DataGrid _updateVisibleRows to skip hidden rows? I think we will miscalculate what is visible and have empty space at the bottom of the DataGrid if enough rows get filtered out.

[Matt Baker]

(In reply to comment #9)
> Comment on attachment 277125 [details]
> [Patch] Proposed Fix
> 
> Do we need to make a change to DataGrid _updateVisibleRows to skip hidden
> rows? I think we will miscalculate what is visible and have empty space at
> the bottom of the DataGrid if enough rows get filtered out.

Just reproduced the issue you described. I'll see if changing _updateVisibleRows fixes it.

[Matt Baker]

Created attachment 277162 [details]
[Video] empty rows after filter

[Matt Baker]

Created attachment 277164 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 277164 [details]
[Patch] Proposed Fix

Rejecting attachment 277164 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 277164, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 199969 = 0db2b28692b6291d8ee66fdbd5a0b09e99d43a44
r199970 = 1a7ecbe733367e42404d6d5eeb41524157119711
r199971 = 246fe5bb32ca14ae29c17ab6d9b1a5b8bc7290b6
r199972 = edb3a07efc64e938eda12dc4186faa0c69ea1602
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/1215103

[Matt Baker]

Created attachment 277214 [details]
[Patch] Proposed Fix

Fixed merge issue

[WebKit Commit Bot]

Comment on attachment 277214 [details]
[Patch] Proposed Fix

Rejecting attachment 277214 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 277214, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/git.webkit.org/WebKit
   162ed14..700a0a6  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 199973 = 162ed14131662ee9058c79f2fc8fd28db05464d8
r199974 = 700a0a6780b1e02e634a5597319e27e1eb061998
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/1215646

[Matt Baker]

Created attachment 277295 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 277295 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 277295

Committed r200067: <http://trac.webkit.org/changeset/200067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.