The default bug view has changed. See this FAQ.

console.log shouldn't display escaped strings

VERIFIED FIXED in Firefox 5

Status

()

Firefox
Developer Tools
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: dherman, Assigned: dherman)

Tracking

Trunk
Firefox 5
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [console-1][has-patch])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
The console.log() function displays its argument as an escaped string, i.e., with double quotes and appropriate backslashes. Most of the time this is extra noise that doesn't help the developer (aka me), and there's no way around it.

If instead it didn't escape its argument, the programmer could always manually escape the string that they pass to console.log(). This would give you the flexibility to do it either way, but default to cleaner and less noisy output.

Thanks,
Dave
(Assignee)

Comment 1

6 years ago
I looked at Firebug: it doesn't just output a string but creates a node with object inspectors and such. So if you do console.log({ foo: "bar" }) you get a clickable object node. But if you do console.log("foo") you get the unescaped string.

A simple approach for now, would be to display strings unescaped, but use formatResult for anything other than a string.

Attaching a patch in a sec.

Dave
(Assignee)

Comment 2

6 years ago
Created attachment 515983 [details] [diff] [review]
simple patch to logConsoleAPIMessage
Attachment #515983 - Flags: review?(gavin.sharp)
Comment on attachment 515983 [details] [diff] [review]
simple patch to logConsoleAPIMessage

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+    let mappedArguments = Array.map(aArguments,
>+                                    function(x) {
>+                                        return (typeof x === "string")
>+                                            ? x
>+                                            : hud.jsterm.formatResult(x);
>+                                    },

Preferred style is ==, typeof(x), and hoisting the function to the top with a name.
(Assignee)

Comment 4

6 years ago
Created attachment 516266 [details] [diff] [review]
updated test cases

Updated patch attempts to fix the failing Mochitests. Running on try server right now. As soon as I get all tests passing I'll r? again.

Dave
Attachment #515983 - Attachment is obsolete: true
Attachment #515983 - Flags: review?(gavin.sharp)
Patch looks fine. If it passes the tryserver tests, then you can ask for review.
(Assignee)

Comment 6

6 years ago
> Patch looks fine. If it passes the tryserver tests, then you can ask for
> review.

Thanks.

FWIW, in test toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_598357_jsterm_output.js, the inputValues array is a little fragile to modify. I think it'd be easier to work with if it were an array of objects instead of an array of arrays:

    let inputValues = [
        { input: "'hello \\nfrom \\rthe \\\"string world!'",
          expectedOutput: '"hello \\nfrom \\rthe \\"string world!"',
          printOutput: "hello \nfrom \rthe \"string world!" },
        { input: "'\xFA\u1E47\u0129\xE7\xF6d\xEA \u021B\u0115\u0219\u0165'",
          expectedOutput: "\"\xFA\u1E47\u0129\xE7\xF6d\xEA \u021B\u0115\u0219\u0165\"",
          printOutput: "\xFA\u1E47\u0129\xE7\xF6d\xEA \u021B\u0115\u0219\u0165" },
        ...
    ];

That way you could also make more properties have defaults, too, like the first boolean parameter:

    let showsPropertyPanel = "showsPropertyPanel" in inputValues[cpos] ?
      inputValues[cpos].showsPropertyPanel : false;

Just to be conservative and try to get the patch landed, I'm not changing this, but I thought I'd mention it.

Dave
Agreed. It initially started as something simple, and it did grow too much. Perhaps that test should be split into simpler tests, actually. That would be nicer in the future.
(In reply to comment #7)
> Agreed. It initially started as something simple, and it did grow too much.
> Perhaps that test should be split into simpler tests, actually. That would be
> nicer in the future.

could one of you file a follow-up bug to do that? Much-appreciated!
(Assignee)

Comment 9

6 years ago
Created attachment 516427 [details] [diff] [review]
off by one level of eval in test #16
Attachment #516266 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
I think I've got this working now, but I seem to have gotten a random orange on tryserver. Can someone try out the patch and tell me if Mochitests pass for them?

Thanks,
Dave
Where's the tryserver link? Philor is great at identifying (and starring) random oranges due to or at least filed as known bugs.

/be
(Assignee)

Comment 12

6 years ago
Revision:
http://hg.mozilla.org/try/rev/0cfee8f53614

Brief log:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1299114178.1299119152.32288.gz

Full log:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1299114178.1299119152.32288.gz&fulltext=1

AFAICT, the failures don't seem relevant to my patch. But I have a terrible time deciphering tinderbox failures, so I don't trust myself.

Dave
(In reply to comment #12)
> Revision:
> http://hg.mozilla.org/try/rev/0cfee8f53614
> 
> Brief log:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1299114178.1299119152.32288.gz
> 
> Full log:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1299114178.1299119152.32288.gz&fulltext=1
> 
> AFAICT, the failures don't seem relevant to my patch. But I have a terrible
> time deciphering tinderbox failures, so I don't trust myself.
> 
> Dave

The failure is starred (bug 618188); you're fine.
(Assignee)

Comment 14

6 years ago
Created attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

Passed all tests on tryserver.

Dave
Assignee: nobody → dherman
Attachment #516427 - Attachment is obsolete: true
Attachment #516673 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #516673 - Flags: review? → review?(gavin.sharp)
Attachment #516673 - Flags: feedback?(mihai.sucan)

Updated

6 years ago
Version: 3.6 Branch → Trunk
Comment on attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

Thanks for the patch Dave! It looks fine, feedback+! All tests pass on my machine as well.
Attachment #516673 - Flags: feedback?(mihai.sucan) → feedback+

Updated

6 years ago
Whiteboard: [console-1][has-patch]
(Assignee)

Comment 16

6 years ago
Gavin,

Can I get a review on this before the window closes for Fx5?

Thanks,
Dave
Comment on attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

nit:

-                                    hud.jsterm);
+    function formatResult(x) {
+        return (typeof(x) == "string") ? x : hud.jsterm.formatResult(x);
+    }

Two too-many spaces on the indent in the return line. 2-spaces only please!

otherwise, r+ from me.
Attachment #516673 - Flags: review+
Comment on attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

rs=me
Attachment #516673 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 524659 [details] [diff] [review]
final version for submission

Just attaching this here so pcwalton can push it for me.

Dave
http://hg.mozilla.org/mozilla-central/rev/2ea06ff58dbe

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 5

Comment 21

6 years ago
Verified on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110421 Firefox/6.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.