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.
Created attachment 184473 [details] Patch
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.
Created attachment 184476 [details] Context menu sample
Created attachment 184477 [details] Context menu sample with filter applied
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
Created attachment 184499 [details] Patch
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.
Created attachment 184996 [details] Patch
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.
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.
Created attachment 185237 [details] Patch
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.
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)
Created attachment 185487 [details] Patch
Comment on attachment 185487 [details] Patch Clearing flags on attachment: 185487 Committed r141269: <http://trac.webkit.org/changeset/141269>
All reviewed patches have been landed. Closing bug.
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);
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 ^