Bug 107813 – Web Inspector: Filters on Console panel

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Dmitry Zvorygin]

Developers are looking for a way to filter console messages. Currently, there is an option to search for the messages but not filter.
Ability to set filters similar to Network panel would be really helpful.
Things are becoming unmanageable as third party libraries are polluting the console with their messages.

[Dmitry Zvorygin]

Created attachment 184473 [details]
Patch

[Pavel Feldman]

Comment on attachment 184473 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Filters on Console panel

Title should be more specific. "Filter out messages from selected scripts in console".

> Source/WebCore/inspector/front-end/ConsoleView.js:40
> +    this._messageUrlFilters = [];

_messageURLFitlers (per WebKit guidelines)

> Source/WebCore/inspector/front-end/ConsoleView.js:42
> +    this._filteredCount = {};

What does this collect?

> Source/WebCore/inspector/front-end/ConsoleView.js:386
> +        if (this._filteredCount[event.data.url])

Please perform cast to ConsoleMessage using annotations. Then access url. Use compile-front-end.py to check for errors.

> Source/WebCore/inspector/front-end/ConsoleView.js:391
> +        if (this._shouldBeVisible(event.data))

So you bump this._filteredCount no matter whether message becomes visible or not. Why?

> Source/WebCore/inspector/front-end/ConsoleView.js:458
> +        var srcElement = event.srcElement;

var sourceElement = event.target.enclosingNodeOrSelfWithClass("console-message");

> Source/WebCore/inspector/front-end/ConsoleView.js:462
> +        var filterSubMenu = contextMenu.appendSubMenuItem("Filter", !(this._messageUrlFilters.length || (srcElement && srcElement.message.url)));

WebInspector.UIString("Fitler"), also add Filter to WebCore/English.lproj/localizedStrings.js if it is not there.

> Source/WebCore/inspector/front-end/ConsoleView.js:468
> +        filterSubMenu.appendItem(WebInspector.UIString("Unhide all"), this._removeMessageUrlFilter.bind(this, -1), !this._messageUrlFilters.length);

add to localized strings

> Source/WebCore/inspector/front-end/ConsoleView.js:487
> +    _addMessageUrlFilter: function(url) {

Here and below: { on the next line, please add closure annotations, URL - all caps.

> Source/WebCore/inspector/front-end/ConsoleView.js:489
> +        var option = document.createElement("option");

Where do you display these options? Please attach a screenshot.

> Source/WebCore/inspector/front-end/ConsoleView.js:519
> +                if (this._shouldBeVisible(visibleMessage)) {

No { } around single line blocks.

> Source/WebCore/inspector/front-end/ConsoleView.js:528
> +                    newVisibleMessages.push(sourceMessage);

This logic requires a test.

> Source/WebCore/inspector/front-end/ConsoleView.js:783
> +    addMessage: function(msg, node)

Please annotate.

> Source/WebCore/inspector/front-end/ConsoleView.js:795
> +                this.messagesElement.insertBefore(element, node);

no {} around single line blocks.

[Dmitry Zvorygin]

Created attachment 184476 [details]
Context menu sample

[Dmitry Zvorygin]

Created attachment 184477 [details]
Context menu sample with filter applied

[Andrey Adaikin]

Comment on attachment 184473 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:497
> +        if (idx == -1)

we try to use "===" everywhere, here and below

> Source/WebCore/inspector/front-end/ConsoleView.js:505
> +    _shouldBeVisible: function(msg) {

@return missing

> Source/WebCore/inspector/front-end/ConsoleView.js:523
> +                    messageElement.parentElement.removeChild(messageElement);

messageElement.removeSelf()

> Source/WebCore/inspector/front-end/ConsoleView.js:794
> +            if (node) {

just this.messagesElement.insertBefore(element, node); - it's cleaner :)

insertBefore already behaves like appendChild if node is null

[Dmitry Zvorygin]

Created attachment 184499 [details]
Patch

[Pavel Feldman]

Comment on attachment 184499 [details]
Patch

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

A bunch of really minor style nits and a request for test.

> Source/WebCore/inspector/front-end/ConsoleView.js:459
> +        var srcElement = event.target.enclosingNodeOrSelfWithClass("console-message");

style: WebKit discourages usage of abbreviated strings. Should be sourceElement.

> Source/WebCore/inspector/front-end/ConsoleView.js:461
> +        var filterSubMenu = contextMenu.appendSubMenuItem("Filter", !(this._messageURLFilters.length || (srcElement && srcElement.message.url)));

UIString("Filter")

> Source/WebCore/inspector/front-end/ConsoleView.js:471
> +            filterSubMenu.appendCheckboxItem(new WebInspector.ParsedURL(this._messageURLFilters[i]).displayName + " (" + this._urlToMessageCount[this._messageURLFilters[i]] + ")", this._removeMessageUrlFilter.bind(this, i), true);

You could use message format: String.sprintf("%s (%d)", displayName, count)

> Source/WebCore/inspector/front-end/ConsoleView.js:502
> +        if (idx == -1)

index (no abbreviations)

> Source/WebCore/inspector/front-end/ConsoleView.js:515
> +    _shouldBeVisible: function(msg)

message

> Source/WebCore/inspector/front-end/ConsoleView.js:520
> +    _updateMessageList: function()

A test?

> Source/WebCore/inspector/front-end/ConsoleView.js:811
> +                this.messagesElement.insertBefore(element, node);

insertBefore(element, null) would work as appendChild and just as fast (see ContainerNode.cpp:241), no need to fork the code here.

[Dmitry Zvorygin]

Created attachment 184996 [details]
Patch

[Dmitry Zvorygin]

Comment on attachment 184499 [details]
Patch

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

>> Source/WebCore/inspector/front-end/ConsoleView.js:461
>> +        var filterSubMenu = contextMenu.appendSubMenuItem("Filter", !(this._messageURLFilters.length || (srcElement && srcElement.message.url)));
> 
> UIString("Filter")

Fixed.

>> Source/WebCore/inspector/front-end/ConsoleView.js:471
>> +            filterSubMenu.appendCheckboxItem(new WebInspector.ParsedURL(this._messageURLFilters[i]).displayName + " (" + this._urlToMessageCount[this._messageURLFilters[i]] + ")", this._removeMessageUrlFilter.bind(this, i), true);
> 
> You could use message format: String.sprintf("%s (%d)", displayName, count)

Done.

>> Source/WebCore/inspector/front-end/ConsoleView.js:502
>> +        if (idx == -1)
> 
> index (no abbreviations)

Fixed.

>> Source/WebCore/inspector/front-end/ConsoleView.js:515
>> +    _shouldBeVisible: function(msg)
> 
> message

Fixed.

>> Source/WebCore/inspector/front-end/ConsoleView.js:520
>> +    _updateMessageList: function()
> 
> A test?

Done.

>> Source/WebCore/inspector/front-end/ConsoleView.js:811
>> +                this.messagesElement.insertBefore(element, node);
> 
> insertBefore(element, null) would work as appendChild and just as fast (see ContainerNode.cpp:241), no need to fork the code here.

Done.

[Pavel Feldman]

Comment on attachment 184996 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:491
> +    _addMessageUrlFilter: function(url)

nit: URL

> Source/WebCore/inspector/front-end/ConsoleView.js:493
> +        this._messageURLFilters.push(url);

nit: as a next change, you'd want to persist this.

> Source/WebCore/inspector/front-end/ConsoleView.js:501
> +    _removeMessageUrlFilter: function(index)

ditto

> Source/WebCore/inspector/front-end/ConsoleView.js:518
> +        return !message.url || this._messageURLFilters.indexOf(message.url) === -1;

This is a linear search which we don't need to do: making filters an object (map from string to boolean) would speed things up.

> Source/WebCore/inspector/front-end/ConsoleView.js:812
> +            this.messagesElement.insertBefore(element, node);

no need for {} around single line block

> LayoutTests/inspector/console/console-filter-test.html:3
> +    <script src="../../http/tests/inspector/inspector-test.js"></script>

nit: we don't indent these - see other tests.

> LayoutTests/inspector/console/console-filter-test.html:7
> +        function log1() {

{ on the next line.

> LayoutTests/inspector/console/console-filter-test.html:12
> +        function onload() {

ditto

> LayoutTests/inspector/console/console-filter-test.html:13
> +            for (var i = 0; i < 10; i++)

missing {}

> LayoutTests/inspector/console/console-filter-test.html:24
> +        function test() {

no indent for top level functions. { on the next line.

> LayoutTests/inspector/console/console-filter-test.html:36
> +            InspectorTest.addResult("=== BEFORE FILTER ===");

You should use runTestSuite to make several tests handling different cases (at least incremental + update)

> LayoutTests/inspector/console/resources/log-source.js:1
> +function log2() {

{ on the next line.

[Dmitry Zvorygin]

Created attachment 185237 [details]
Patch

[Dmitry Zvorygin]

Comment on attachment 184996 [details]
Patch

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

>> Source/WebCore/inspector/front-end/ConsoleView.js:491
>> +    _addMessageUrlFilter: function(url)
> 
> nit: URL

Done

>> Source/WebCore/inspector/front-end/ConsoleView.js:493
>> +        this._messageURLFilters.push(url);
> 
> nit: as a next change, you'd want to persist this.

Done.

>> Source/WebCore/inspector/front-end/ConsoleView.js:501
>> +    _removeMessageUrlFilter: function(index)
> 
> ditto

Done

>> Source/WebCore/inspector/front-end/ConsoleView.js:518
>> +        return !message.url || this._messageURLFilters.indexOf(message.url) === -1;
> 
> This is a linear search which we don't need to do: making filters an object (map from string to boolean) would speed things up.

Done.

>> Source/WebCore/inspector/front-end/ConsoleView.js:812
>> +            this.messagesElement.insertBefore(element, node);
> 
> no need for {} around single line block

Done.

>> LayoutTests/inspector/console/console-filter-test.html:3
>> +    <script src="../../http/tests/inspector/inspector-test.js"></script>
> 
> nit: we don't indent these - see other tests.

Done.

>> LayoutTests/inspector/console/console-filter-test.html:12
>> +        function onload() {
> 
> ditto

Done.

>> LayoutTests/inspector/console/console-filter-test.html:13
>> +            for (var i = 0; i < 10; i++)
> 
> missing {}

Done.

>> LayoutTests/inspector/console/console-filter-test.html:24
>> +        function test() {
> 
> no indent for top level functions. { on the next line.

Done.

>> LayoutTests/inspector/console/console-filter-test.html:36
>> +            InspectorTest.addResult("=== BEFORE FILTER ===");
> 
> You should use runTestSuite to make several tests handling different cases (at least incremental + update)

Done.

>> LayoutTests/inspector/console/resources/log-source.js:1
>> +function log2() {
> 
> { on the next line.

Done.

[Pavel Feldman]

Comment on attachment 185237 [details]
Patch

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

Almost good to go.

> Source/WebCore/inspector/front-end/ContextMenu.js:64
> +    get disabled()

Please do not use getters and setters. setEnabled: function(enabled)

[Dmitry Zvorygin]

Created attachment 185487 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185487 [details]
Patch

Clearing flags on attachment: 185487

Committed r141269: <http://trac.webkit.org/changeset/141269>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrey Adaikin]

Comment on attachment 185487 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:823
> +            this.messagesElement.insertBefore(element, node);

Source/WebCore/inspector/front-end/ConsoleView.js:823: WARNING - actual parameter 2 of Element.prototype.insertBefore does not match formal parameter
found   : (Node|null|undefined)
required: (Node|null)
            this.messagesElement.insertBefore(element, node);

[Andrey Adaikin]

Comment on attachment 185487 [details]
Patch

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

> Source/WebCore/inspector/front-end/ContextMenu.js:106
> +     * @param {function} handler

Source/WebCore/inspector/front-end/ContextMenu.js:106: WARNING - Bad type annotation. missing opening (
     * @param {function} handler
                       ^