Bug 152038 – Web Inspector: console.count() shouldn't show a colon in front of a number

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 266968 [details]
[Image] Bug

Steps:
1. Run console.count() in the console

Expected:
"1"

Actual:
": 1"

[Radar WebKit Bug Importer]

<rdar://problem/23816590>

[Johan K. Jensen]

Created attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

[Build Bot]

Comment on attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

Attachment 268646 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/675349

New failing tests:
inspector/console/console-api.html

[Build Bot]

Created attachment 268648 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

Attachment 268646 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/675350

New failing tests:
inspector/console/console-api.html

[Build Bot]

Created attachment 268649 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

Attachment 268646 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/675346

New failing tests:
inspector/console/console-api.html

[Build Bot]

Created attachment 268650 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

Comment on attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

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

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:178
> +    if (title.isEmpty())

What if the argument is all whitespace?

[Blaze Burg]

Comment on attachment 268646 [details]
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`.

Looks like a good change, but the corresponding test needs to be rebaselined and edited to cover this change. The test is LayoutTests/inspector/console/console-api.html.

Please add the following cases (at least) to the test, to cover the counting functionality and the handling of whitespace/empty arguments.

console.count() // second time
console.count("")
console.count("    ")
console.count("") // second time
console.count("    ") // second time
console.count("counter title")
console.count("counter title") // second time

[Blaze Burg]

(In reply to comment #10)
> Comment on attachment 268646 [details]
> Not quite sure if the empty string (`console.count("")`) should show `: 1`
> or if it should be equivalent to `console.count()`.
> 
> Looks like a good change, but the corresponding test needs to be rebaselined
> and edited to cover this change. The test is
> LayoutTests/inspector/console/console-api.html.
> 
> Please add the following cases (at least) to the test, to cover the counting
> functionality and the handling of whitespace/empty arguments.
> 
> console.count() // second time
> console.count("")
> console.count("    ")
> console.count("") // second time
> console.count("    ") // second time
> console.count("counter title")
> console.count("counter title") // second time

We should probably also add test cases for passing null, undefined, object, number to console.count() to document what it does.

[Blaze Burg]

(In reply to comment #2)
> Created attachment 268646 [details]
> Not quite sure if the empty string (`console.count("")`) should show `: 1`
> or if it should be equivalent to `console.count()`.

I think we should match what Chrome / Firefox does. This is mainly implemented in Web Inspector for compatibility with other devtools.

[Johan K. Jensen]

Both Firefox and Chrome has `console.count()` and `console.count('')` be equivalent (i.e. the count would be 2 after running those commands), while any additional whitespace to the title-parameter would create new counters.
Firefox also writes "<no label>: x" for counters without a title or with the empty title (but not for whitespace).
Should WebKit also show "<no label>: x" for those two cases? (And if so, should it be localized? And how would I do that?) Or should it just show "x"?

[Blaze Burg]

(In reply to comment #13)
> Both Firefox and Chrome has `console.count()` and `console.count('')` be
> equivalent (i.e. the count would be 2 after running those commands), while
> any additional whitespace to the title-parameter would create new counters.
> Firefox also writes "<no label>: x" for counters without a title or with the
> empty title (but not for whitespace).
> Should WebKit also show "<no label>: x" for those two cases? (And if so,
> should it be localized? And how would I do that?) Or should it just show "x"?

Yes, let's match what they do. I think it looks kind of dumb (do you have a better idea?) but showing nothing is even worse.

Don't worry about localizing this; none of the console messages are currently localized. They should be localized, but that should be addressed in a separate bug.

[Johan K. Jensen]

Created attachment 268697 [details]
Patch

[Joseph Pecoraro]

I've been wondering for a while now if we should just drop console.count. Last I checked, no browser implements a sufficiently useful version of it. They all track just line number, so in minified sources it is completely useless as all counts() are considered the same line.

Has this grown in popularity somewhere? Is there a good use case for it?

[Blaze Burg]

Comment on attachment 268697 [details]
Patch

r=me, This is looking great. Thanks for updating the test!

[Nikita Vasilyev]

(In reply to comment #16)
> I've been wondering for a while now if we should just drop console.count.
> Last I checked, no browser implements a sufficiently useful version of it.
> They all track just line number, so in minified sources it is completely
> useless as all counts() are considered the same line.
> 
> Has this grown in popularity somewhere? Is there a good use case for it?

I've seen people use console.count("method name") for some basic profiling, e.g.:

function foo() {
    console.count("foo")
    ...
}

function bar() {
    console.count("bar")
    ...
}

We could draw charts for console.count:

foo: 42 ****************
bar: 10 ****

I'd use it.

I don't know where would we put this in the UI though.

[WebKit Commit Bot]

Comment on attachment 268697 [details]
Patch

Clearing flags on attachment: 268697

Committed r194887: <http://trac.webkit.org/changeset/194887>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.