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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Console
VERIFIED FIXED
5 months ago
3 months ago

People

(Reporter: domfarolino, Assigned: tromey)

Tracking

({dev-doc-needed})

52 Branch
Firefox 55
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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

Updated

5 months ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Console
Ever confirmed: true
Summary: console.count() incorrect default counter label → console.count() should use "default" as the default counter label to follow the latest spec for console
(Assignee)

Comment 1

5 months ago
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.
(Assignee)

Comment 2

5 months ago
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.
Assignee: nobody → ttromey
Comment hidden (mozreview-request)
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.
Attachment #8859149 - Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 6

4 months ago
> 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

4 months ago
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.
(Assignee)

Comment 8

4 months ago
> 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.
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.
(Assignee)

Comment 10

4 months ago
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.
(Assignee)

Comment 11

4 months ago
After reading https://heycam.github.io/webidl/#es-DOMString I saw the light.
However I see this is being done in bug 1357473.
Comment hidden (mozreview-request)
Blocks: 1357473
Tom, I would like to land the patches of bug 1357473. Do you mind if I land yours together with those?

Comment 14

4 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fe2d7d463f
implement new console.count semantics; r=baku
(Assignee)

Comment 15

4 months ago
That wasn't ready to go in.
(Assignee)

Comment 16

4 months ago
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
(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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1fe2d7d463f
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 19

4 months ago
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]
Keywords: dev-doc-needed

Comment 20

3 months ago
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]

Updated

3 months ago
QA Whiteboard: [bugday-20170517]

Comment 21

3 months ago
As per Comment 19 & Comment 20, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.