1346326 - console.count() should use "default" as the default counter label to follow the latest spec for console

Status: VERIFIED FIXED

(DevTools :: Console, defect)

Description

[Dominic Farolino [:domfarolino]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.100 Safari/537.36

Steps to reproduce:

The spec for console (https://console.spec.whatwg.org) was recently changed such that the default counter label is "default", though technically FF was not compliant before the change too.

Open the Developer Tools and in the console type:

console.count()
console.count(undefined)
console.count("default")


Actual results:

Results:

console.count()
> <no label>: 1
console.count(undefined)
> undefined: 1
console.count("default")
> default: 1


Expected results:

Expected results:

console.count()
> default: 1
console.count(undefined)
> default: 2
console.count("default")
> default: 3

As per https://github.com/whatwg/console/issues/88 and https://github.com/whatwg/console/pull/91 the default parameter should be the string "default", and all of the following calls should trigger the same counter.

This issue is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1270636

Comment 1

[Tom Tromey :tromey]

It seems simple to change in Console.webidl, but I wonder if this is likely to break anything,
because the argument's type would be changing from "any..." to DOMString.
A different fix could be done in Console.cpp perhaps.

Comment 2

[Tom Tromey :tromey]

It turned out to be far simpler to do the work in Console.cpp, because then I didn't
have to change how Console::Method works.  Not sure if this idea is too ugly though.

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1346326 - implement new console.count semantics;

Bug 1346326 points out that the console.count spec has changed to

  void count(optional DOMString label = "default");

This patch implements the same behavior, but does not change the webidl,
on the theory that perhaps something relies on the current "any..."
type.

Review commit: https://reviewboard.mozilla.org/r/131194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131194/

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8859149 [details]
Bug 1346326 - implement new console.count semantics;

https://reviewboard.mozilla.org/r/131194/#review133734

Looks good to me with this comment applied.

::: dom/console/Console.cpp:2116
(Diff revision 1)
>  
> -  nsAutoString key;
>    nsAutoString label;
>  
> -  if (!aArguments.IsEmpty()) {
> +  if (aArguments.IsEmpty()) {
> +    label.AssignLiteral("default");

Can you please add (together with the other #define...):

#define COUNT_DEFAULT_LABEL "default"

and use them here.

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8859149 [details]
Bug 1346326 - implement new console.count semantics;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131194/diff/1-2/

Comment 6

[Dominic Farolino [:domfarolino]]

> Can you please add (together with other #define...)

By "other #define" do you mean other default definitions that appear elsewhere in the spec (https://console.spec.whatwg.org/#console-namespace) such as #define TIME_DEFAULT_LABEL = "default"

I'm not too familiar with the codebase (yet) but I'm just curious if that's what you means since that seems helpful.

Comment 7

[Domenic Denicola]

If this patch does not intend to update the Web IDL to DOMString behavior, then please make sure to also fix https://bugzilla.mozilla.org/show_bug.cgi?id=1346336 which mandates that behavior.

Comment 8

[Tom Tromey :tromey]

> If this patch does not intend to update the Web IDL to DOMString behavior

It wasn't obvious to me that this change would be safe to do.
This is really my first foray into patches that touch on web standards though.
So, I wouldn't mind extra guidance.

Comment 9

[Andrea Marchesini [:baku]]

Tom, let's keep the 2 bugs separate. Your patch looks good to me.
We have several issues with throwing exceptions. Let's work on them in bug 1346336.

Comment 10

[Tom Tromey :tromey]

I thought perhaps I'd see what chromium is using here, on the theory that if it is using
DOMString then perhaps changing is safe enough.  However, after much searching, I couldn't
find the code.  So, I'm giving up on that and going back to trying to clean up the
terrible try results from this patch.

Comment 11

[Tom Tromey :tromey]

After reading https://heycam.github.io/webidl/#es-DOMString I saw the light.
However I see this is being done in bug 1357473.

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8859149 [details]
Bug 1346326 - implement new console.count semantics;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131194/diff/2-3/

Comment 13

[Andrea Marchesini [:baku]]

Tom, I would like to land the patches of bug 1357473. Do you mind if I land yours together with those?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fe2d7d463f
implement new console.count semantics; r=baku

Comment 15

[Tom Tromey :tromey]

That wasn't ready to go in.

Comment 16

[Tom Tromey :tromey]

For the record I think it still needs this patch.
If there's a try failure perhaps we can put it in quickly to avoid backout.

diff --git a/devtools/client/webconsole/test/browser_webconsole_count.js b/devtools/client/webconsole/test/browser_webconsole_count.js
index 4a02f0e..d1b3725 100644
--- a/devtools/client/webconsole/test/browser_webconsole_count.js
+++ b/devtools/client/webconsole/test/browser_webconsole_count.js
@@ -37,13 +37,6 @@ function test() {
         severity: SEVERITY_LOG
       });
     });
-    messages.push({
-      name: "Three local counts with no label and count=1",
-      text: "default: 1",
-      category: CATEGORY_WEBDEV,
-      severity: SEVERITY_LOG,
-      count: 3
-    });
     yield waitForMessages({
       webconsole: hud,
       messages: messages
@@ -57,7 +50,9 @@ function test() {
       "start",
       "console.count() testcounter: 5",
       "console.count() testcounter: 6",
-      "end"
+      "default: 5",
+      "default: 6",
+      "end",
     ].forEach(function (msg) {
       messages.push({
         text: msg,
@@ -65,13 +60,6 @@ function test() {
         severity: SEVERITY_LOG
       });
     });
-    messages.push({
-      name: "Two external counts with no label and count=1",
-      text: "default: 1",
-      category: CATEGORY_WEBDEV,
-      severity: SEVERITY_LOG,
-      count: 2
-    });
     yield waitForMessages({
       webconsole: hud,
       messages: messages

Comment 17

[Andrea Marchesini [:baku]]

(In reply to Tom Tromey :tromey from comment #16)
> For the record I think it still needs this patch.
> If there's a try failure perhaps we can put it in quickly to avoid backout.

The test has been fixed in my patches as well with a similar approach.
The patch looked good on try and it's green on m-i as well: 
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8d71c8281a83b28a93d54d452b1533f29646e00a

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d1fe2d7d463f

Comment 19

[Sayed [:LiquidO2]]

I have reproduced this bug following the STR from comment #0 with nightly 55.0a1 (2017-03-11) (64-bit) on Manjaro 17 (64bit).

I can verify that this bug is fixed in Latest Nightly 55.0a1

Build ID 	20170426100344
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]

Comment 20

[Md. Rahimul Islam]

I have successfully reproduced the bug in firefox Nightly 55.0a1 (2017-03-10) (32-bit) with windows 7 (32 bit) 

Verified as fixed with latest nightly 55.0a1 (2017-05-17) (32-bit)

Build ID:   20170517030204
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0
[bugday-20170516]

Comment 21

[Md. Rahimul Islam]

As per Comment 19 & Comment 20, I am marking this bug as verified fixed.