Bug 210672 - Web Inspector: removing a `WI.TreeElement` in a `WI.NavigationSidebar` doesn't check if the `WI.TreeOutline` still matches the current filter
Summary: Web Inspector: removing a `WI.TreeElement` in a `WI.NavigationSidebar` doesn'...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-17 14:30 PDT by Devin Rousso
Modified: 2020-04-20 12:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.53 KB, patch)
2020-04-17 14:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2020-04-17 17:52 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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'.