Web Inspector: requests filtering in network tab

[Pavel Chadnov]

Created attachment 156327 [details]
Screenshot

In network tab search is replaced with filtering

[Pavel Chadnov]

Created attachment 156328 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156328 [details]
Patch

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

[Pavel Chadnov]

Created attachment 156384 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156384 [details]
Patch

Please make sure you address all the review comments. Also please rebaseline your patch so that bots could try it.

[Pavel Chadnov]

Created attachment 156677 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156677 [details]
Patch

Please make sure you address all the review comments.

[Pavel Chadnov]

Created attachment 156685 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156685&action=review

> Source/WebCore/ChangeLog:3
> +        A new feature added: filtering in the network panel.

This line should be equal to the bug summary.

> Source/WebCore/ChangeLog:5
> +        Reviewed by NOBODY (OOPS!).

Please add a blank line before this line.

> Source/WebCore/ChangeLog:6
> +

Please add a change description here. See previous changes's ChangeLogs for the reference

> Source/WebCore/inspector/front-end/FilterController.js:5
> + * modification, are permitted provided that the following conditions are met: *

Please fix formatting of the license text, there should be new lines before '*'. It should look the same as in the other files.

> Source/WebCore/inspector/front-end/FilterController.js:33
> +    this._element.cellSpacing = 0;

Please remove this.

> Source/WebCore/inspector/front-end/FilterController.js:35
> +    // Column 1

Please remove these comments.

> Source/WebCore/inspector/front-end/NetworkPanel.js:-754
> -

Please revert this change.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1757
> +        if (!this._parentView._hiddenCategories.all && !this._parentView._filteredOutRequests.get(this._request))

It is enough to look at _filteredOutRequests once.

if (this._parentView._filteredOutRequests.get(this._request))
    return true;
if (!this._parentView._hiddenCategories.all)
    return false;
return this._request.type.name() in this._parentView._hiddenCategories;

[Pavel Chadnov]

Created attachment 156696 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156696&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests (OOPS!).

Please remove this line (and 1 blank line).

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

Here and everywhere: we put open bracket { to the next line after the function name.

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

Please add compiler annotations to this function.

> Source/WebCore/inspector/front-end/NetworkPanel.js:443
> +        for(var i = 0; i < this._dataGrid.rootNode().children.length; ++i){

Please add space after for and a space before bracket.

> Source/WebCore/inspector/front-end/NetworkPanel.js:447
> +            if (node.element.children[0].innerText.indexOf(content) === -1){

Please use createPlainTextSearchRegex like it is done in performSearch.
Also a space before bracket please.

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

Please add compiler annotations to this function.

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

Please add compiler annotations to this function.

[Pavel Chadnov]

Created attachment 156897 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156897 [details]
Patch

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.

[Pavel Chadnov]

Created attachment 156911 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156911 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156911&action=review

> Source/WebCore/inspector/front-end/NetworkPanel.js:-1554
> -        // of a specific event. If startAtZero is set, then this is useless, and we

Why did this change?
Please revert weird formatting changes.

[Pavel Chadnov]

Created attachment 156920 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 156920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156920&action=review

> Source/WebCore/inspector/front-end/NetworkPanel.js:1169
> +            node.element.removeStyleClass("filtered-out");

I'd move this below:

if (node._request.displayName.match(filterRegExp)) 
    node.element.removeStyleClass("filtered-out");
else {
    node.element.addStyleClass("filtered-out");
    this._filteredOutRequests.put(this._requests[i], true);
}

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

Annotations are still missing.

[Vsevolod Vlasov]

Comment on attachment 156920 [details]
Patch

As discussed offline let's get rid of FilterController and implement filtering as a checkbox on search pane.

[Pavel Chadnov]

Created attachment 158281 [details]
Patch

[Pavel Chadnov]

Created attachment 158293 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 158293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158293&action=review

Considering the scope of this patch the search controller change looks too complicated to me.
I think this complexity is the result of hiding inspector footer and showing again each time the checkbox state is changed.
We should update already shown footer instead.

> Source/WebCore/inspector/front-end/InspectorView.js:85
> +            WebInspector.searchController.panelChanged();

cancelSearch call should be enough.

[Pavel Chadnov]

Created attachment 158338 [details]
Patch

[Pavel Chadnov]

Created attachment 158360 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 158360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158360&action=review

> Source/WebCore/ChangeLog:8
> +        New feature: there are both search bar and filter bar in network tabw. One can swich them using checkbox "filtering".

Please fix typos.

> Source/WebCore/inspector/front-end/SearchController.js:46
> +    this._firstRowElement.createChild("td");

Why is this needed?

> Source/WebCore/inspector/front-end/SearchController.js:57
> +  

Extra blank line.

> Source/WebCore/inspector/front-end/SearchController.js:74
> +    this._filterInputElement.addEventListener("keydown", this._onKeyDown.bind(this), true);

keydown handler has some search specific logic that should be treated carefully.

> Source/WebCore/inspector/front-end/SearchController.js:79
> +    this._secondRowElement.createChild("td");

Why is this needed?

> Source/WebCore/inspector/front-end/SearchController.js:125
> +  

Extra blank line.

> Source/WebCore/inspector/front-end/SearchController.js:136
> +    this._searchOrFilterQuery = "";

unused

> Source/WebCore/inspector/front-end/SearchController.js:255
> +        this._performSearch(this._searchInputElement.value);

Please remove this line, we don't want to change search behavior.

> Source/WebCore/inspector/front-end/SearchController.js:432
> +            this._performSearch(this._filterInputElement.value, false, false);

Please use this._searchInputElement for consistency.

[Pavel Chadnov]

Created attachment 158532 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 158532 [details]
Patch

As discussed please add match highlighting for filtering.
Please attach a screenshot for the new implementation also.

[Pavel Chadnov]

Created attachment 158779 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 158779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158779&action=review

> Source/WebCore/inspector/front-end/NetworkPanel.js:1072
> +        this._highlightMatchedRequests([], false, "");

"" doesn't look like a regexp to me. Please annotate new methods to make sure we catch such errors.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1106
> +        if (regExp.test(""))

I don't think this is needed.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1155
> +                this._toggleLargerRequests();

This should be in highlight... method because it's needed both for search and filtering.

[Pavel Chadnov]

Created attachment 158792 [details]
Patch

[Pavel Chadnov]

Created attachment 158807 [details]
Screenshot

[Pavel Chadnov]

Created attachment 158808 [details]
Screenshot

[Vsevolod Vlasov]

Comment on attachment 158792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158792&action=review

> Source/WebCore/inspector/front-end/NetworkPanel.js:1072
> +        this._highlightMatchedRequests([], false, createPlainTextSearchRegex(""));

Let's always use ...ForSearch method for search.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1136
> +    _highlightNthMatchedRequestForSearch: function(matchedRequestIndex, reveal, regExp)

Please remove regExp parameter here, for search it is always this._searchRegExp

[Pavel Chadnov]

Created attachment 158841 [details]
Patch

[Pavel Chadnov]

Created attachment 158848 [details]
Patch

[Pavel Chadnov]

Created attachment 159048 [details]
Patch

[Pavel Chadnov]

Created attachment 159057 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159057 [details]
Patch

Clearing flags on attachment: 159057

Committed r125879: <http://trac.webkit.org/changeset/125879>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.