[webkit-reviews] review denied: [Bug 93090] Web Inspector: requests filtering in network tab : [Attachment 156897] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Pavel Chadnov
<chadnov at google.com>'s request for review:
Bug 93090: Web Inspector: requests filtering in network tab
https://bugs.webkit.org/show_bug.cgi?id=93090

Attachment 156897: Patch
https://bugs.webkit.org/attachment.cgi?id=156897&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156897&action=review


> Source/WebCore/inspector/front-end/FilterController.js:38
> +    this._filterControlElement = this._element.createChild("span",
"toolbar-filter-control");

This element is redundant.

> Source/WebCore/inspector/front-end/FilterController.js:59
> +	   this._performFilter('');

Please use double quotes.

> Source/WebCore/inspector/front-end/FilterController.js:60
> +	   this._filterInputElement.value = '';

Please use double quotes.

> Source/WebCore/inspector/front-end/FilterController.js:63
> +    _performFilter: function(query) 

Compiler annotations please.

> Source/WebCore/inspector/front-end/FilterController.js:66
> +	   if (typeof currentPanel.performFilter === 'function')

Please use double quotes.

> Source/WebCore/inspector/front-end/FilterController.js:78
> +	       return false;

return;

> Source/WebCore/inspector/front-end/FilterController.js:81
> +	   if (isEnterKey(event)) {

No need to handle enter since we perform filtering on input event anyway.

> Source/WebCore/inspector/front-end/FilterController.js:104
> +	   var isMac = WebInspector.isMac();

isMac is used only once, please inline.

> Source/WebCore/inspector/front-end/FilterController.js:107
> +		   var isFindKey = event.metaKey && !event.ctrlKey &&
!event.altKey && !event.shiftKey;

isFilterKey

> Source/WebCore/inspector/front-end/NetworkPanel.js:447
> +	       this._filteredOutRequests.put(this._requests[i], true);

Just clear the map before the loop and fill it with non matching requests
below.
Otherwise it looks confusing: you remove "hidden" style and mark request as
filtered out at the same time.

> Source/WebCore/inspector/front-end/NetworkPanel.js:448
> +	       if (!_match)

_match is never defined and is probably redundant.
Also wrong indent here and below.

> Source/WebCore/inspector/front-end/NetworkPanel.js:450
> +		   node.element.addStyleClass("hidden");

I wouldn't use global hidden class here since it says nothing about the reason
why the row is not visible. I would add .network-log-grid.data-grid
tr.filtered-out instead

> Source/WebCore/inspector/front-end/NetworkPanel.js:1174
> +	   this.performFilter('');

Please use double quotes.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1181
> +	   this._filterByContent(query);

What is the difference between performFilter and filterByContent? There is no
need to extract one from another.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1450
> +    performFilter: function(query){

Compiler annotations please.