Bug 93090 - Web Inspector: requests filtering in network tab
Summary: Web Inspector: requests filtering in network tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-03 04:52 PDT by Pavel Chadnov
Modified: 2012-08-17 03:55 PDT (History)
11 users (show)

See Also:


Attachments
Screenshot (92.24 KB, image/png)
2012-08-03 04:52 PDT, Pavel Chadnov
no flags Details
Patch (20.40 KB, patch)
2012-08-03 04:56 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (25.32 KB, patch)
2012-08-03 08:14 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (26.40 KB, patch)
2012-08-06 06:16 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (16.94 KB, patch)
2012-08-06 07:06 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2012-08-06 08:00 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (16.91 KB, patch)
2012-08-07 02:12 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (25.70 KB, patch)
2012-08-07 04:36 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (18.49 KB, patch)
2012-08-07 05:51 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (18.73 KB, patch)
2012-08-14 03:33 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (18.72 KB, patch)
2012-08-14 04:38 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (16.43 KB, patch)
2012-08-14 08:34 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (15.91 KB, patch)
2012-08-14 10:17 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2012-08-15 02:39 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (21.81 KB, patch)
2012-08-16 05:00 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (22.05 KB, patch)
2012-08-16 06:05 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Screenshot (85.87 KB, image/png)
2012-08-16 07:06 PDT, Pavel Chadnov
no flags Details
Screenshot (94.49 KB, image/png)
2012-08-16 07:07 PDT, Pavel Chadnov
no flags Details
Patch (21.94 KB, patch)
2012-08-16 09:46 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (21.81 KB, patch)
2012-08-16 10:00 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (21.81 KB, patch)
2012-08-17 01:53 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff
Patch (21.90 KB, patch)
2012-08-17 02:32 PDT, Pavel Chadnov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Chadnov 2012-08-03 04:52:05 PDT
Created attachment 156327 [details]
Screenshot

In network tab search is replaced with filtering
Comment 1 Pavel Chadnov 2012-08-03 04:56:12 PDT
Created attachment 156328 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-08-03 05:20:52 PDT
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
Comment 3 Pavel Chadnov 2012-08-03 08:14:33 PDT
Created attachment 156384 [details]
Patch
Comment 4 Vsevolod Vlasov 2012-08-06 00:35:32 PDT
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.
Comment 5 Pavel Chadnov 2012-08-06 06:16:38 PDT
Created attachment 156677 [details]
Patch
Comment 6 Vsevolod Vlasov 2012-08-06 06:28:41 PDT
Comment on attachment 156677 [details]
Patch

Please make sure you address all the review comments.
Comment 7 Pavel Chadnov 2012-08-06 07:06:11 PDT
Created attachment 156685 [details]
Patch
Comment 8 Vsevolod Vlasov 2012-08-06 07:33:47 PDT
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;
Comment 9 Pavel Chadnov 2012-08-06 08:00:00 PDT
Created attachment 156696 [details]
Patch
Comment 10 Vsevolod Vlasov 2012-08-06 08:52:40 PDT
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.
Comment 11 Pavel Chadnov 2012-08-07 02:12:42 PDT
Created attachment 156897 [details]
Patch
Comment 12 Vsevolod Vlasov 2012-08-07 02:30:58 PDT
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.
Comment 13 Pavel Chadnov 2012-08-07 04:36:24 PDT
Created attachment 156911 [details]
Patch
Comment 14 Vsevolod Vlasov 2012-08-07 05:39:04 PDT
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.
Comment 15 Pavel Chadnov 2012-08-07 05:51:40 PDT
Created attachment 156920 [details]
Patch
Comment 16 Vsevolod Vlasov 2012-08-07 06:10:47 PDT
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.
Comment 17 Vsevolod Vlasov 2012-08-09 01:46:48 PDT
Comment on attachment 156920 [details]
Patch

As discussed offline let's get rid of FilterController and implement filtering as a checkbox on search pane.
Comment 18 Pavel Chadnov 2012-08-14 03:33:52 PDT
Created attachment 158281 [details]
Patch
Comment 19 Pavel Chadnov 2012-08-14 04:38:51 PDT
Created attachment 158293 [details]
Patch
Comment 20 Vsevolod Vlasov 2012-08-14 06:52:56 PDT
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.
Comment 21 Pavel Chadnov 2012-08-14 08:34:20 PDT
Created attachment 158338 [details]
Patch
Comment 22 Pavel Chadnov 2012-08-14 10:17:10 PDT
Created attachment 158360 [details]
Patch
Comment 23 Vsevolod Vlasov 2012-08-14 23:51:25 PDT
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.
Comment 24 Pavel Chadnov 2012-08-15 02:39:01 PDT
Created attachment 158532 [details]
Patch
Comment 25 Vsevolod Vlasov 2012-08-16 01:12:20 PDT
Comment on attachment 158532 [details]
Patch

As discussed please add match highlighting for filtering.
Please attach a screenshot for the new implementation also.
Comment 26 Pavel Chadnov 2012-08-16 05:00:14 PDT
Created attachment 158779 [details]
Patch
Comment 27 Vsevolod Vlasov 2012-08-16 05:13:21 PDT
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.
Comment 28 Pavel Chadnov 2012-08-16 06:05:43 PDT
Created attachment 158792 [details]
Patch
Comment 29 Pavel Chadnov 2012-08-16 07:06:16 PDT
Created attachment 158807 [details]
Screenshot
Comment 30 Pavel Chadnov 2012-08-16 07:07:36 PDT
Created attachment 158808 [details]
Screenshot
Comment 31 Vsevolod Vlasov 2012-08-16 09:19:48 PDT
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
Comment 32 Pavel Chadnov 2012-08-16 09:46:29 PDT
Created attachment 158841 [details]
Patch
Comment 33 Pavel Chadnov 2012-08-16 10:00:45 PDT
Created attachment 158848 [details]
Patch
Comment 34 Pavel Chadnov 2012-08-17 01:53:45 PDT
Created attachment 159048 [details]
Patch
Comment 35 Pavel Chadnov 2012-08-17 02:32:09 PDT
Created attachment 159057 [details]
Patch
Comment 36 WebKit Review Bot 2012-08-17 03:55:12 PDT
Comment on attachment 159057 [details]
Patch

Clearing flags on attachment: 159057

Committed r125879: <http://trac.webkit.org/changeset/125879>
Comment 37 WebKit Review Bot 2012-08-17 03:55:20 PDT
All reviewed patches have been landed.  Closing bug.