Bug 152038 - Web Inspector: console.count() shouldn't show a colon in front of a number
Summary: Web Inspector: console.count() shouldn't show a colon in front of a number
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: EasyFix, GoodFirstBug, InRadar
Depends on:
Blocks:
 
Reported: 2015-12-08 22:18 PST by Nikita Vasilyev
Modified: 2016-01-11 23:39 PST (History)
15 users (show)

See Also:


Attachments
[Image] Bug (2.46 KB, image/png)
2015-12-08 22:18 PST, Nikita Vasilyev
no flags Details
Not quite sure if the empty string (`console.count("")`) should show `: 1` or if it should be equivalent to `console.count()`. (1.66 KB, patch)
2016-01-10 08:41 PST, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (950.71 KB, application/zip)
2016-01-10 09:29 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (822.90 KB, application/zip)
2016-01-10 09:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (866.88 KB, application/zip)
2016-01-10 09:35 PST, Build Bot
no flags Details
Patch (7.39 KB, patch)
2016-01-11 10:51 PST, Johan K. Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-12-08 22:18:51 PST
Created attachment 266968 [details]
[Image] Bug

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

Expected:
"1"

Actual:
": 1"
Comment 1 Radar WebKit Bug Importer 2015-12-08 22:19:02 PST
<rdar://problem/23816590>
Comment 2 Johan K. Jensen 2016-01-10 08:41:06 PST
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()`.
Comment 3 Build Bot 2016-01-10 09:29:16 PST
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
Comment 4 Build Bot 2016-01-10 09:29:19 PST
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
Comment 5 Build Bot 2016-01-10 09:32:20 PST
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
Comment 6 Build Bot 2016-01-10 09:32:23 PST
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
Comment 7 Build Bot 2016-01-10 09:35:47 PST
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
Comment 8 Build Bot 2016-01-10 09:35:50 PST
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
Comment 9 Brian Burg 2016-01-10 11:27:21 PST
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?
Comment 10 Brian Burg 2016-01-10 11:31:18 PST
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
Comment 11 Brian Burg 2016-01-10 11:32:17 PST
(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.
Comment 12 Brian Burg 2016-01-10 11:33:02 PST
(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.
Comment 13 Johan K. Jensen 2016-01-10 13:19:01 PST
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"?
Comment 14 Brian Burg 2016-01-11 10:28:06 PST
(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.
Comment 15 Johan K. Jensen 2016-01-11 10:51:32 PST
Created attachment 268697 [details]
Patch
Comment 16 Joseph Pecoraro 2016-01-11 14:38:20 PST
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?
Comment 17 Brian Burg 2016-01-11 22:53:10 PST
Comment on attachment 268697 [details]
Patch

r=me, This is looking great. Thanks for updating the test!
Comment 18 Nikita Vasilyev 2016-01-11 23:16:12 PST
(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.
Comment 19 WebKit Commit Bot 2016-01-11 23:39:31 PST
Comment on attachment 268697 [details]
Patch

Clearing flags on attachment: 268697

Committed r194887: <http://trac.webkit.org/changeset/194887>
Comment 20 WebKit Commit Bot 2016-01-11 23:39:36 PST
All reviewed patches have been landed.  Closing bug.