[webkit-reviews] review granted: [Bug 178071] Web Inspector: Network Tab - Filter resources based on URL / Text Content : [Attachment 323156] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 9 10:17:32 PDT 2017
Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 178071: Web Inspector: Network Tab - Filter resources based on URL / Text
Content
https://bugs.webkit.org/show_bug.cgi?id=178071
Attachment 323156: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=323156&action=review
--- Comment #7 from Brian Burg <bburg at apple.com> ---
Comment on attachment 323156
--> https://bugs.webkit.org/attachment.cgi?id=323156
[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!
More information about the webkit-reviews
mailing list