| Summary: | Web Inspector: Filters on Console panel | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dmitry Zvorygin <zvorygin> | ||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | apavlov, keishi, loislo, mkwst+watchlist, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Dmitry Zvorygin
2013-01-24 05:25:15 PST
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 ^ |