Closed Bug 637801 Opened 9 years ago Closed 9 years ago

console.log shouldn't display escaped strings

Categories

(DevTools :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 5

People

(Reporter: dherman, Assigned: dherman)

Details

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

Attachments

(2 files, 3 obsolete files)

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
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
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.
Attached patch updated test cases (obsolete) — Splinter Review
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.
> 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!
Attachment #516266 - Attachment is obsolete: true
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
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.
Passed all tests on tryserver.

Dave
Assignee: nobody → dherman
Attachment #516427 - Attachment is obsolete: true
Attachment #516673 - Flags: review?
Attachment #516673 - Flags: review? → review?(gavin.sharp)
Attachment #516673 - Flags: feedback?(mihai.sucan)
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+
Whiteboard: [console-1][has-patch]
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+
Just attaching this here so pcwalton can push it for me.

Dave
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 5
Verified on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110421 Firefox/6.0a1
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.