637801 - console.log shouldn't display escaped strings

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Dave Herman [:dherman]]

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

Comment 1

[Dave Herman [:dherman]]

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

Comment 2

[Dave Herman [:dherman]]

Attachment: simple patch to logConsoleAPIMessage

Comment 3

[Patrick Walton (:pcwalton)]

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.

Comment 4

[Dave Herman [:dherman]]

Attachment: 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

Comment 5

[Mihai Sucan [:msucan]]

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

Comment 6

[Dave Herman [:dherman]]

> 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

Comment 7

[Mihai Sucan [:msucan]]

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.

Comment 8

[Rob Campbell [:rc] (:robcee)]

(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!

Comment 9

[Dave Herman [:dherman]]

Attachment: off by one level of eval in test #16

Comment 10

[Dave Herman [:dherman]]

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

Comment 11

[Brendan Eich [:brendan]]

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

/be

Comment 12

[Dave Herman [:dherman]]

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

Comment 13

[Patrick Walton (:pcwalton)]

(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.

Comment 14

[Dave Herman [:dherman]]

Attachment: removed tryserver mumbo jumbo

Passed all tests on tryserver.

Dave

Comment 15

[Mihai Sucan [:msucan]]

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.

Comment 16

[Dave Herman [:dherman]]

Gavin,

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

Thanks,
Dave

Comment 17

[Rob Campbell [:rc] (:robcee)]

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.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

rs=me

Comment 19

[Dave Herman [:dherman]]

Attachment: final version for submission

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

Dave

Comment 20

[Patrick Walton (:pcwalton)]

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

Comment 21

[George Carstoiu]

Verified on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110421 Firefox/6.0a1