[webkit-reviews] review denied: [Bug 107813] Web Inspector: Filters on Console panel : [Attachment 184996] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 28 09:34:23 PST 2013
Pavel Feldman <pfeldman at chromium.org> has denied Dmitry Zvorygin
<zvorygin at chromium.org>'s request for review:
Bug 107813: Web Inspector: Filters on Console panel
https://bugs.webkit.org/show_bug.cgi?id=107813
Attachment 184996: Patch
https://bugs.webkit.org/attachment.cgi?id=184996&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.
More information about the webkit-reviews
mailing list