[webkit-reviews] review denied: [Bug 93090] Web Inspector: requests filtering in network tab : [Attachment 156328] 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 156328: Patch
https://bugs.webkit.org/attachment.cgi?id=156328&action=review

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


Please provide a ChangeLog and fix style.

> Source/WebCore/WebCore.gypi:11
> +	       'Modules/geolocation/Geoposition.h',

Why did it move?

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:2520
> +

Please remove these extra lines

> Source/WebCore/inspector/front-end/FilterController.js:2
> + * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved. Copyright
(C)

2012 only. Please use Google copyright. See SettingsScreen.js for an example.

> Source/WebCore/inspector/front-end/FilterController.js:33
> +WebInspector.FilterController = function() {

Wrong indent.

> Source/WebCore/inspector/front-end/FilterController.js:34
> +  this._element = document.createElement("table");

I don't think you need to use table for such a simple component.

> Source/WebCore/inspector/front-end/FilterController.js:39
> +  this._secondRowElement = this._element.createChild("tr", "hidden");

Please remove this second row element, you don't need it here.

> Source/WebCore/inspector/front-end/FilterController.js:47
> +  this._filterInputElement.addEventListener("mousedown", null, false);

What's the point?

> Source/WebCore/inspector/front-end/FilterController.js:60
> +

Remove this line

> Source/WebCore/inspector/front-end/FilterController.js:70
> +    

One line between methods, please.

> Source/WebCore/inspector/front-end/FilterController.js:79
> +    _onKeyDown : function(event) {

No space before :

> Source/WebCore/inspector/front-end/FilterController.js:83
> +	  
WebInspector.setCurrentFocusElement(WebInspector.previousFocusElement());

I don't think you saved previous focus element, so it makes no sense to set it
back.
You should save it in showFilterField

> Source/WebCore/inspector/front-end/FilterController.js:100
> +    showFilterField : function() {

Ditto

> Source/WebCore/inspector/front-end/FilterController.js:111
> +	 if (event.keyIdentifier === "U+0046") {

What is that? Please add an inline comment.

> Source/WebCore/inspector/front-end/NetworkPanel.js:51
> +    this._isRequestVisible = new Map();

This field needs a better name.
I would actually call it _filteredOutRequests and store only requests that were
filtered out.

> Source/WebCore/inspector/front-end/NetworkPanel.js:441
> +    _filterByContent: function(content){

Weird indent (4 spaces indent in webkit)

> Source/WebCore/inspector/front-end/NetworkPanel.js:444
> +	   node.element.removeStyleClass("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:446
> +	   if (node.element.children[0].innerText.indexOf(content)==-1){

Please add spaces near ==.
Also we use === in WebInspector.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1143
> +//	 performSearch: function(searchQuery)

Please remove commented out code.

> Source/WebCore/inspector/front-end/SearchController.js:158
> +	 if (typeof WebInspector.inspectorView.currentPanel().performSearch !==
"function")

Weird indent