Last Comment Bug 922208 - Add console.count
: Add console.count
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 30
Assigned To: Dennis Schubert [:denschub]
:
: Brian Grinstead [:bgrins]
Mentors:
Depends on:
Blocks: 922204
  Show dependency treegraph
 
Reported: 2013-09-30 10:31 PDT by Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
Modified: 2014-03-19 08:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed console.count() implementation, rev. 1 (14.98 KB, patch)
2013-12-22 05:38 PST, Dennis Schubert [:denschub]
no flags Details | Diff | Splinter Review
console.count() in Firebug, Chrome Devtools and Firefox Devtools (159.92 KB, image/png)
2013-12-22 05:40 PST, Dennis Schubert [:denschub]
no flags Details
Testcase used to create the screenshot of console.count() implementations (1.66 KB, application/zip)
2013-12-23 09:31 PST, Dennis Schubert [:denschub]
no flags Details
Proposed console.count() implementation, rev. 2-WIP 1 (18.17 KB, patch)
2014-02-04 17:07 PST, Dennis Schubert [:denschub]
no flags Details | Diff | Splinter Review
Proposed console.count() implementation, rev. 2 (23.93 KB, patch)
2014-02-12 15:26 PST, Dennis Schubert [:denschub]
mihai.sucan: review+
Details | Diff | Splinter Review
Proposed console.count() implementation, rev. 3 (24.24 KB, patch)
2014-02-15 12:44 PST, Dennis Schubert [:denschub]
no flags Details | Diff | Splinter Review

Description User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2013-09-30 10:31:47 PDT
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
Comment 1 User image Dennis Schubert [:denschub] 2013-12-22 05:38:52 PST
Created attachment 8351005 [details] [diff] [review]
Proposed console.count() implementation, rev. 1

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.
Comment 2 User image Dennis Schubert [:denschub] 2013-12-22 05:40:01 PST
Created attachment 8351006 [details]
console.count() in Firebug, Chrome Devtools and Firefox Devtools

Just for the record: A comparing screenshot of different console.count() implementations is attached - left: Firebug; middle: Chrome; right: proposed Firefox Devtools implementation.
Comment 3 User image Mihai Sucan [:msucan] 2013-12-23 09:16:27 PST
Dennis, can you please attach the test case files?
Comment 4 User image Dennis Schubert [:denschub] 2013-12-23 09:31:53 PST
Created attachment 8351230 [details]
Testcase used to create the screenshot of console.count() implementations

Sure, Mihai. All the files (the HTML and two JS files) are attached.
Comment 5 User image Mihai Sucan [:msucan] 2013-12-23 10:53:39 PST
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.
Comment 6 User image Dennis Schubert [:denschub] 2014-02-04 17:07:01 PST
Created attachment 8370470 [details] [diff] [review]
Proposed console.count() implementation, rev. 2-WIP 1

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().
Comment 7 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2014-02-04 17:14:24 PST
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.
Comment 8 User image Mihai Sucan [:msucan] 2014-02-05 03:19:19 PST
(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?
Comment 9 User image J. Ryan Stinnett [:jryans] (use ni?) 2014-02-05 09:23:27 PST
(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.
Comment 10 User image Mihai Sucan [:msucan] 2014-02-05 09:51:35 PST
(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.
Comment 11 User image Dennis Schubert [:denschub] 2014-02-05 09:57:47 PST
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.
Comment 12 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2014-02-05 11:19:46 PST
If it got removed, then sorry for the interruption! Full steam ahead!
Comment 13 User image Dennis Schubert [:denschub] 2014-02-12 15:26:41 PST
Created attachment 8375161 [details] [diff] [review]
Proposed console.count() implementation, rev. 2

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?
Comment 14 User image Mihai Sucan [:msucan] 2014-02-14 09:45:10 PST
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.
Comment 15 User image Dennis Schubert [:denschub] 2014-02-15 12:44:48 PST
Created attachment 8376749 [details] [diff] [review]
Proposed console.count() implementation, rev. 3

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. :)
Comment 16 User image Mihai Sucan [:msucan] 2014-02-15 13:06:58 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=8dffc18408df
Comment 17 User image Mihai Sucan [:msucan] 2014-02-17 13:04:36 PST
Try push is green. Landed in fx-team.

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

Thank you!
Comment 18 User image Phil Ringnalda (:philor) 2014-02-17 20:58:41 PST
https://hg.mozilla.org/mozilla-central/rev/86565c3d9a5d
Comment 19 User image Will Bamberg [:wbamberg] 2014-03-18 21:16:54 PDT
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!
Comment 20 User image Mihai Sucan [:msucan] 2014-03-19 03:11:48 PDT
(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!

Note You need to log in before you can comment on or make changes to this bug.