Bug 210672

Summary:

Web Inspector: removing a `WI.TreeElement` in a `WI.NavigationSidebar` doesn't check if the `WI.TreeOutline` still matches the current filter

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none

Description Devin Rousso 2020-04-17 14:30:23 PDT

# STEPS TO REPRODUCE
1. inspect any page with a JavaScript resource
2. go to the Sources Tab
3. filter for that JavaScript resource
 => the Breakpoints section should say "No Filter Results"
4. set a breakpoint in that JavaScript resource
 => the Breakpoints section should update to show the new breakpoint
5. delete the breakpoint added in step 4

## EXPECTED

The Breakpoints section should show "No Filter Results".

## ACTUAL

The Breakpoints section is empty.

Comment 1 Devin Rousso 2020-04-17 14:30:55 PDT

Created attachment 396800 [details]
Patch

Comment 2 Joseph Pecoraro 2020-04-17 17:15:35 PDT

Comment on attachment 396800 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:634
> +    _handleTreeElementRemoved(event)
> +    {
> +        this._checkForEmptyFilterResults();

I wonder how much time is spent in `_checkForEmptyFilterResults` during bulk modifications (adds or removes). That could easily be made more efficient if needed.

Comment 3 Devin Rousso 2020-04-17 17:52:23 PDT

Created attachment 396820 [details]
Patch

well one optimization would be to only look inside the `WI.TreeOutline` that actually changed :P

Comment 4 EWS 2020-04-20 11:02:08 PDT

Committed r260375: <https://trac.webkit.org/changeset/260375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396820 [details].

Comment 5 Radar WebKit Bug Importer 2020-04-20 11:03:15 PDT

<rdar://problem/62066377>

Comment 6 BJ Burg 2020-04-20 12:59:40 PDT

Comment on attachment 396820 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:531
> +    _checkForEmptyFilterResults(treeOutline)

Naming nit:

I would not assume from the name that this updates the UI with the No Results placeholder. Nor can I understand what the parameter is. It replaces the per-outline function, but doesn't mention outline in the name.

I'd prefer _updateOutlineForEmptyFilterResultsIfNeeded(), maybe dropping IfNeeded, if needed. :)

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:572
> +        // All top level tree elements are hidden, so filtering hid everything. Show a message.

Nit on the original line: 'hid' is weird, I would use 'omitted'.