Summary: | Web Inspector: Network Tab - Filter resources based on URL / Text Content | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||
Component: | Web Inspector | Assignee: | 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
Joseph Pecoraro
2017-10-08 19:46:58 PDT
Created attachment 323152 [details]
[IMAGE] Filter placeholder
Created attachment 323153 [details]
[IMAGE] Filter active
Created attachment 323154 [details]
[IMAGE] Filter no results
Created attachment 323155 [details]
[IMAGE] Filter text content example
Created attachment 323156 [details]
[PATCH] Proposed Fix
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 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. Created attachment 323208 [details]
[IMAGE] Filter no results - updated
Created attachment 323209 [details]
[PATCH] Proposed Fix
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. |