[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