Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Description • 11 years ago

https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consolecountlabel

> Writes the the number of times that count() has been invoked at the same
> line and with the same label.
>
> In the following example count() is invoked each time the login() function
> is invoked.
>
>     function test () {
>         function login(user) {
>             console.count("Login called for " + user);
>         } 
>         login("brian");
>         login("paul");
>         login("paul");
>     }
>     test();
>
>     > Login called for brian: 1
>       Login called for paul: 1
>       Login called for paul: 2

	Dennis Schubert [:denschub] Assignee
	Comment 1 • 11 years ago

Before writing the attached patched, I did some testing to see how console.count() works in Firebug and Chrome to determinate the best way of implementing it.

Firebug does care about file names and line numbers. Counters with the same label but a different file name or line number will not affect each other. Chrome does _not_ care about it. Only the label itself is used to distinguish the counters.

As discussed with Mihai, the patch attached implements the Chrome way, i.e. using only the label to distinguish the counters.

	Dennis Schubert [:denschub] Assignee
	Comment 2 • 11 years ago

Just for the record: A comparing screenshot of different console.count() implementations is attached - left: Firebug; middle: Chrome; right: proposed Firefox Devtools implementation.

	Mihai Sucan [:msucan]
	Comment 3 • 11 years ago

Dennis, can you please attach the test case files?

	Dennis Schubert [:denschub] Assignee
	Comment 4 • 11 years ago

Sure, Mihai. All the files (the HTML and two JS files) are attached.

	Mihai Sucan [:msucan]
	Comment 5 • 11 years ago

Comment on attachment 8351005 [details] [diff] [review]
Proposed console.count() implementation, rev. 1

Review of attachment 8351005 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Dennis! This is good work and it's close to an r+.

Review comments follow below:

::: browser/devtools/webconsole/test/browser_webconsole_count.js
@@ +9,5 @@
> +function test() {
> +  addTab(TEST_URI);
> +  browser.addEventListener("load", function onLoad() {
> +    browser.removeEventListener("load", onLoad, true);
> +    openConsole(null, consoleOpened);

openConsole() now also returns a promise, please use that instead of the callback approach.

Please use Task.spawn() for organizing this test file. See how the web console output tests look - eg. see  the new browser_webconsole_output_events.js file.

Read more about Task.jsm: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm

@@ +22,5 @@
> +      category: CATEGORY_WEBDEV,
> +      severity: SEVERITY_LOG,
> +    },
> +    {
> +      text: /foo: [1-3]/,

Please be more explicit about these expected messages. Add one for each, and the count numbers.

::: browser/devtools/webconsole/test/test-console-count.html
@@ +7,5 @@
> +    -->
> +    <meta charset="utf-8">
> +    <title>console.count() test</title>
> +    <script type="text/javascript">
> +      function test() {

Needs more testing. Different functions, <script> tags and an external js file. Similar to the attached test case you submitted.

::: browser/devtools/webconsole/webconsole.js
@@ +1257,5 @@
>          clipboardText = body;
>          break;
>        }
>  
> +      case "count": {

This is looking good, but it fallbacks to the old way of adding messages to the output (it calls createMessageNode()). That approach is deprecated. Please use the ConsoleOutput API. See how we handle the cases for "assert", "debug", "log", etc.

Move this code into the Messages.ConsoleGeneric object.

::: dom/base/ConsoleAPI.js
@@ +525,5 @@
> +   *        holds the current count.
> +   **/
> +  increaseCounter: function CA_increaseCounter(aLabel) {
> +    if (!aLabel) {
> +      return;

There's a strict warning: CA_increaseCounter() does not always return a value.

@@ +527,5 @@
> +  increaseCounter: function CA_increaseCounter(aLabel) {
> +    if (!aLabel) {
> +      return;
> +    }
> +    if (!this.counterRegistry.has(aLabel)) {

Before using the label in any way, convert it to a string.

@@ +528,5 @@
> +    if (!aLabel) {
> +      return;
> +    }
> +    if (!this.counterRegistry.has(aLabel)) {
> +      this.counterRegistry.set(aLabel, 1);

Before adding a counter please check the map size. Limit the number of maximum page counters. Similar to how we have a MAX_PAGE_TIMERS limit.

::: dom/tests/browser/browser_ConsoleAPITests.js
@@ +262,5 @@
>    win.console.assert(false, "message");
>    yield undefined;
>  
> +  expect("count", "label");
> +  win.console.count("label");

Need more tests here as well. Check counter value is incremented. Also test a different label.

	Dennis Schubert [:denschub] Assignee
	Comment 6 • 10 years ago

Thanks for your very fast feedback, Mihai, and sorry for me not responding. I've not abandoned this issue, I'm just totally overloaded with other work. :)

There is a _work in progress_ patch attached, in which some of your comments are already addressed. However, I didn't expand the test cases so far and there is still one issue left which I haven't looked into so far: the error reporting in case of an exceeded counter limit is not working, see the comment in console-output.js, Messages.ConsoleGeneric().

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 7 • 10 years ago

Sorry for only catching this now, but the wording is pretty clear to me that it should be consider calls on different lines to be different counts. We should try and follow the API defined there, and I expect Chrome's devtools will eventually as well since Paul Irish et all are involved.

(In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> https://github.com/DeveloperToolsWG/console-object/blob/master/api.
> md#consolecountlabel
> 
> > Writes the the number of times that count() has been invoked at the same
> > line and with the same label.

	Mihai Sucan [:msucan]
	Comment 8 • 10 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Sorry for only catching this now, but the wording is pretty clear to me that
> it should be consider calls on different lines to be different counts. We
> should try and follow the API defined there, and I expect Chrome's devtools
> will eventually as well since Paul Irish et all are involved.

No problem. However, this approach makes it harder to count the same "thing" (label) from different places in your code. The current Chrome implementation made sense to me. Is there any reason why lines matter here?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 9 • 10 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Sorry for only catching this now, but the wording is pretty clear to me that
> it should be consider calls on different lines to be different counts. We
> should try and follow the API defined there, and I expect Chrome's devtools
> will eventually as well since Paul Irish et all are involved.
> 
> (In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> > https://github.com/DeveloperToolsWG/console-object/blob/master/api.
> > md#consolecountlabel
> > 
> > > Writes the the number of times that count() has been invoked at the same
> > > line and with the same label.

Is that your own paraphrase of the document?  I don't find that text on the page you're linking to...  Instead it says:

* Writes the the number of times that count() has been invoked with the same label.
* If no label is provided then the script url and line number of the console.count statement is used for associating the counter with console.count invocation.

So, my read of the document is that the line only matters if no label is provided.

	Mihai Sucan [:msucan]
	Comment 10 • 10 years ago

(In reply to J. Ryan Stinnett [:jryans] from comment #9)
> ...
> 
> Is that your own paraphrase of the document?  I don't find that text on the
> page you're linking to...  Instead it says:
> 
> * Writes the the number of times that count() has been invoked with the same
> label.
> * If no label is provided then the script url and line number of the
> console.count statement is used for associating the counter with
> console.count invocation.
> 
> So, my read of the document is that the line only matters if no label is
> provided.

Agreed. I also re-read that now. This is making a lot more sense. Thanks Ryan. 

Nick, is this understanding of the document fine with you? I'd like Dennis to know what he needs to update in his patch.

	Dennis Schubert [:denschub] Assignee
	Comment 11 • 10 years ago

Apparently, the line-dependent behavior got removed at 31st dec (see https://github.com/DeveloperToolsWG/console-object/commit/cc3a01a7b877db10b1c6ebd9751f0247b031363a) and the default label got added at 2nd feb (see https://github.com/DeveloperToolsWG/console-object/commit/f6627358dafa3cd427d3d2a8570e7b519265f937) so I'd go for updating the implementation accordingly if there are no objections.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 12 • 10 years ago

If it got removed, then sorry for the interruption! Full steam ahead!

	Dennis Schubert [:denschub] Assignee
	Comment 13 • 10 years ago

Finally... As discussed, I reimplemented some parts to match the changed specs on GitHub. In addition, I extended the tests to cover all ways of using console.count(), so this patch is ready to review. Mihai, could you please take a look at it?

	Mihai Sucan [:msucan]
	Comment 14 • 10 years ago

Comment on attachment 8375161 [details] [diff] [review]
Proposed console.count() implementation, rev. 2

Review of attachment 8375161 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r+ with the comments below addressed. Thank you for the updated patch.

::: browser/devtools/webconsole/console-output.js
@@ +1080,5 @@
>    };
> +  switch (packet.level) {
> +    case "count": {
> +      let counter = packet.counter;
> +      if (counter) {

Is it ever possible to have ConsoleGeneric invoked with a packet that doesn't have a counter? webconsole.js doesn't allow it. We don't need to check here.

::: browser/devtools/webconsole/test/browser_webconsole_count.js
@@ +11,5 @@
> +  addTab(TEST_URI);
> +  browser.addEventListener("load", function onLoad() {
> +    browser.removeEventListener("load", onLoad, true);
> +    Task.spawn(runner);
> +  }, true);

You can replace addTab() and the event listener with: yield loadTab(url). See browser_webconsole_console_trace_duplicates.js for an example.

::: browser/devtools/webconsole/webconsole.js
@@ +1265,5 @@
>      }
>  
>      // Release object actors for arguments coming from console API methods that
>      // we ignore their arguments.
>      switch (level) {

Please add "count" to this switch as well. We ignore the arguments, so we need to release any object actors.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +119,5 @@
>  # is the number of milliseconds.
>  timeEnd=%1$S: %2$Sms
>  
> +# LOCALIZATION NOTE (noCounterLabel): this string is used to display
> +# count-messages with no label provided

nit: period at the end.

@@ +128,5 @@
>  Autocomplete.blank=  <- no result
>  
>  maxTimersExceeded=The maximum allowed number of timers in this page was exceeded.
>  
> +maxCountersExceeded=The maximum allowed number of counters in this page was exceeded.

Add a localization note.

::: dom/base/ConsoleAPI.js
@@ +536,5 @@
> +   **/
> +  increaseCounter: function CA_increaseCounter(aFrame, aLabel) {
> +    let key = null, label = null;
> +    if (!aLabel) {
> +      key = aFrame.filename + ":" + aFrame.lineNumber.toString();

toString() is not needed, it happens when you do string concat.

@@ +538,5 @@
> +    let key = null, label = null;
> +    if (!aLabel) {
> +      key = aFrame.filename + ":" + aFrame.lineNumber.toString();
> +    } else {
> +      label = aLabel.toString();

This can throw for labels that are objects without toString().

Better to do:

let key = null, label = null;
try {
  label = key = aLabel + "";
} catch (ex) { }

if (!key) {
  ... set the key as you do now ...
}

@@ +541,5 @@
> +    } else {
> +      label = aLabel.toString();
> +      key = label;
> +    }
> +    let counter = {};

nit: let counter = null; you override the object anyway.

@@ +551,5 @@
> +    } else {
> +      counter = this.counterRegistry.get(key);
> +      counter.count += 1;
> +    }
> +    this.counterRegistry.set(key, counter);

You should only need to call set() once, when you add the counter.

	Dennis Schubert [:denschub] Assignee
	Comment 15 • 10 years ago

Thanks, Mihai! I implemented your feedback in the patch attached. Besides your feedback, I've also changed the curly brace positions in browser_webconsole_count.js to match the style guides and the rest of the source. :)

	Mihai Sucan [:msucan]
	Comment 16 • 10 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8dffc18408df

	Mihai Sucan [:msucan]
	Comment 17 • 10 years ago

Try push is green. Landed in fx-team.

https://hg.mozilla.org/integration/fx-team/rev/86565c3d9a5d

Thank you!

	Phil Ringnalda (:philor)
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/86565c3d9a5d

	Will Bamberg [:wbamberg]
	Comment 19 • 10 years ago

Mihai, do these look all right to you?
-> https://developer.mozilla.org/en-US/docs/Web/API/console.count
-> https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Log_messages

Thanks!

	Mihai Sucan [:msucan]
	Comment 20 • 10 years ago

(In reply to Will Bamberg [:wbamberg] from comment #19)
> Mihai, do these look all right to you?
> -> https://developer.mozilla.org/en-US/docs/Web/API/console.count
> -> https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Log_messages

Yes. Thank you very much!