[Webkit-unassigned] [Bug 93090] Web Inspector: requests filtering in network tab

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93090


Vsevolod Vlasov <vsevik at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #156328|review?                     |review-
               Flag|                            |




--- Comment #2 from Vsevolod Vlasov <vsevik at chromium.org>  2012-08-03 05:20:52 PST ---
(From update of attachment 156328)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.