Bug 178071

Summary: Web Inspector: Network Tab - Filter resources based on URL / Text Content
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, bburg, buildbot, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[IMAGE] Filter placeholder
none
[IMAGE] Filter active
none
[IMAGE] Filter no results
none
[IMAGE] Filter text content example
none
[PATCH] Proposed Fix
bburg: review+, bburg: commit-queue-
[IMAGE] Filter no results - updated
none
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2017-10-08 19:46:58 PDT
Network Tab - Filter resources based on URL / Text Content

Should be able to filter resources in the table based on their name or contents.

Examples scenarios:

  • filter only those in a particular domain
  • filter to the resources that have "id: 12345" in their content
  • filter to all "min.js" resources
  • filter to the resources with "?t" in their URL
Comment 1 Joseph Pecoraro 2017-10-08 19:47:29 PDT
<rdar://problem/34071562>
Comment 2 Joseph Pecoraro 2017-10-08 19:54:31 PDT
Created attachment 323152 [details]
[IMAGE] Filter placeholder
Comment 3 Joseph Pecoraro 2017-10-08 19:54:43 PDT
Created attachment 323153 [details]
[IMAGE] Filter active
Comment 4 Joseph Pecoraro 2017-10-08 19:54:56 PDT
Created attachment 323154 [details]
[IMAGE] Filter no results
Comment 5 Joseph Pecoraro 2017-10-08 19:55:13 PDT
Created attachment 323155 [details]
[IMAGE] Filter text content example
Comment 6 Joseph Pecoraro 2017-10-08 19:55:36 PDT
Created attachment 323156 [details]
[PATCH] Proposed Fix
Comment 7 BJ Burg 2017-10-09 10:17:32 PDT
Comment on attachment 323156 [details]
[PATCH] Proposed Fix

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

r=me but please try EWS again. I have a few questions.

> Source/WebInspectorUI/ChangeLog:46
> +        warning in the Debugger tab when breakpoints are disabled.

Nice touch.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:68
> +    resourceForIdentifier(requestIdentifier)

Nit: resourceForRequestIdentifier?

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:66
> +        return this._inputField.placeholder;

Nice cleanup.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:99
> +    right: 0;

Please extract 0 to var(content-view-warning-banner-offset-start). Do we use warning banner inside other content views? This could be a generic rule + ContentView feature.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:80
> +        this._textFilterNavigationItem.filterBar.placeholder = WI.UIString("Filter Full URL and Text");

This string seems a little weird to me, but I can't come up with an obviously better replacement.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:655
> +        PageAgent.searchInResource(frame.id, resource.url, searchQuery, isCaseSensitive, isRegex, resource.requestIdentifier, (error, searchResults) => {

Oh dear, so many parameters x_x

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:657
> +                return;

What does this early return do?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:660
> +                return;

Please console.error() or WI.reportInternalError() so we can catch bugs here.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:787
> +    _positionEmptyFilterMessage()

I like how this is its own function.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1036
> +        this._textFilterNavigationItem.filterBar.inputField.value = null; // Get the placeholder to show again.

:|

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1090
> +        this._hideResourceDetailView();

Did you try this out? I'm not sure which behavior is less annoying, just reading this.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1113
> +        // For those we should provide more custom filters.

Is this something the content view does or the Table/delegate/datasource?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1133
> +            if (!error) {

Ditto about logging errors. This could be a console.error and early return.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:92
> +    resetToDefault()

I really wish UI tests were possible yet!
Comment 8 Joseph Pecoraro 2017-10-09 13:11:18 PDT
Comment on attachment 323156 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/ChangeLog:46
>> +        warning in the Debugger tab when breakpoints are disabled.
> 
> Nice touch.

New patch will have a new appearance for this.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:657
>> +                return;
> 
> What does this early return do?

This is if a new search was performed before this async result returned.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:660
>> +                return;
> 
> Please console.error() or WI.reportInternalError() so we can catch bugs here.

This can happen. I know of 1 case where we error (main resource on reloads...) and not having search results just means no results were found.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1036
>> +        this._textFilterNavigationItem.filterBar.inputField.value = null; // Get the placeholder to show again.
> 
> :|

I'm actually going to move this into FilterBar so that FilterBar can reset a little bit of its state.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1090
>> +        this._hideResourceDetailView();
> 
> Did you try this out? I'm not sure which behavior is less annoying, just reading this.

When users are showing a detail view, changing filters in general doesn't make much sense so I don't expect users to hit this much in practice. But in any case I don't want us to be showing Detail views for a resource that is not selected (filtered out). This is a simple solution.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1113
>> +        // For those we should provide more custom filters.
> 
> Is this something the content view does or the Table/delegate/datasource?

All filtering happens in NetworkTableContentView. Changing a filter modifies the dataSource data and asks the table to reload.
Comment 9 Joseph Pecoraro 2017-10-09 13:22:43 PDT
Created attachment 323208 [details]
[IMAGE] Filter no results - updated
Comment 10 Joseph Pecoraro 2017-10-09 13:22:56 PDT
Created attachment 323209 [details]
[PATCH] Proposed Fix
Comment 11 Joseph Pecoraro 2017-10-09 13:24:50 PDT
Comment on attachment 323209 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:124
> +    clear()
> +    {
> +        this._inputField.value = "";
> +        this._inputField.value = null; // Get the placeholder to show again.
> +        this._lastFilterValue = this.filters;
> +    }

This was a small change. Reseting lastFilterValue means if you search "asdf", Clear Filters, and search "asdf" again that it will work. Previously it wasn't.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:79
> +.content-view.network .empty-content-placeholder {

And this is the style change for No Filter Results no longer being a banner.
Comment 12 Joseph Pecoraro 2017-10-09 13:38:34 PDT
<https://trac.webkit.org/r223065>