Web Inspector: Debugger sidebar should have a filter button for breakpoints

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

Currently the debugger sidebar always shows any and all debuggable resources. This should be optional and set by a button on the filter bar similar to the Xcode style.

<rdar://problem/19524415>

[Jonathan Wells]

Created attachment 248836 [details]
[PATCH] Added filter button.

[Jonathan Wells]

Created attachment 248837 [details]
[SCREENSHOT] Filter is off.

[Jonathan Wells]

Created attachment 248838 [details]
[SCREENSHOT] Filter is on.

[Jonathan Wells]

Created attachment 248839 [details]
[SCREENSHOT] Button and text filter applied.

[Jonathan Wells]

Look at the screenshots. We probably want another icon for the button.

[Joseph Pecoraro]

Comment on attachment 248836 [details]
[PATCH] Added filter button.

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

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:57
>      var breakpointsImage, pauseImage, resumeImage, stepOverImage, stepIntoImage, stepOutImage;

Style: You can now move these var's inline (60-65).

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:108
> +        return treeElement instanceof WebInspector.ResourceTreeElement && !treeElement.hasChildren;

This condition seems the opposite of what I would want it to be.

I'd want the filter to be a predicate that says "true = show, false = hide". Because it is much easier to say "this is what I want to show" then "this is everything that should be hidden".

However, when reading the condition, "ResourceTreeElement && !children" = true => show, is not the case.

> Source/WebInspectorUI/UserInterface/Views/FilterBarButton.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2015.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:320
> +            // If the filter function returns true for a treeElement, that element should be hidden.
> +            if (filterFunction(treeElement))
> +                return false;

This is what I think should be flipped.

[Jonathan Wells]

Comment on attachment 248836 [details]
[PATCH] Added filter button.

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

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:108
>> +        return treeElement instanceof WebInspector.ResourceTreeElement && !treeElement.hasChildren;
> 
> This condition seems the opposite of what I would want it to be.
> 
> I'd want the filter to be a predicate that says "true = show, false = hide". Because it is much easier to say "this is what I want to show" then "this is everything that should be hidden".
> 
> However, when reading the condition, "ResourceTreeElement && !children" = true => show, is not the case.

Yeah I struggled with this too. I'll change it.

>> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:320
>> +                return false;
> 
> This is what I think should be flipped.

Right, that works as long as the filters functions return true of things you want to keep, as you suggested. Will change.

[Jonathan Wells]

Created attachment 248988 [details]
[PATCH] with JoePeck feedback

[Jonathan Wells]

Created attachment 249014 [details]
[PATCH] Corrected patch based on feedback.

[Timothy Hatcher]

Comment on attachment 249014 [details]
[PATCH] Corrected patch based on feedback.

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

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:119
> +    };
> +    this.filterBar.addFilterBarButton("debugger-show-resources-with-children-only", showResourcesWithBreakpointsOnlyFilterFunction, false, WebInspector.UIString("Show only resources with breakpoints."), WebInspector.UIString("Show resources with and without breakpoints."), platformImagePath("Breakpoints.svg"), 15, 15);

Newline between these lines? Pass true to enable this by default.

[Jonathan Wells]

Committed r181872: <http://trac.webkit.org/changeset/181872>