Last Comment Bug 637801 - console.log shouldn't display escaped strings
: console.log shouldn't display escaped strings
Status: VERIFIED FIXED
[console-1][has-patch]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 5
Assigned To: Dave Herman [:dherman]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-01 11:08 PST by Dave Herman [:dherman]
Modified: 2011-04-22 04:47 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple patch to logConsoleAPIMessage (1.53 KB, patch)
2011-03-01 11:59 PST, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
updated test cases (6.34 KB, patch)
2011-03-02 07:45 PST, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
off by one level of eval in test #16 (6.79 KB, patch)
2011-03-02 15:19 PST, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
removed tryserver mumbo jumbo (6.75 KB, patch)
2011-03-03 12:22 PST, Dave Herman [:dherman]
gavin.sharp: review+
rcampbell: review+
mihai.sucan: feedback+
Details | Diff | Splinter Review
final version for submission (6.75 KB, patch)
2011-04-08 10:21 PDT, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review

Description Dave Herman [:dherman] 2011-03-01 11:08:25 PST
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] 2011-03-01 11:57:02 PST
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] 2011-03-01 11:59:16 PST
Created attachment 515983 [details] [diff] [review]
simple patch to logConsoleAPIMessage
Comment 3 Patrick Walton (:pcwalton) 2011-03-01 12:03:38 PST
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] 2011-03-02 07:45:44 PST
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
Comment 5 Mihai Sucan [:msucan] 2011-03-02 08:05:32 PST
Patch looks fine. If it passes the tryserver tests, then you can ask for review.
Comment 6 Dave Herman [:dherman] 2011-03-02 08:35:18 PST
> 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] 2011-03-02 08:38:30 PST
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) 2011-03-02 08:40:22 PST
(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] 2011-03-02 15:19:03 PST
Created attachment 516427 [details] [diff] [review]
off by one level of eval in test #16
Comment 10 Dave Herman [:dherman] 2011-03-02 21:17:24 PST
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] 2011-03-03 11:27:09 PST
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] 2011-03-03 11:33:29 PST
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) 2011-03-03 11:35:02 PST
(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] 2011-03-03 12:22:58 PST
Created attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

Passed all tests on tryserver.

Dave
Comment 15 Mihai Sucan [:msucan] 2011-03-09 09:20:42 PST
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] 2011-04-03 17:24:52 PDT
Gavin,

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

Thanks,
Dave
Comment 17 Rob Campbell [:rc] (:robcee) 2011-04-06 11:39:48 PDT
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: gavin@gavinsharp.com] 2011-04-06 14:04:00 PDT
Comment on attachment 516673 [details] [diff] [review]
removed tryserver mumbo jumbo

rs=me
Comment 19 Dave Herman [:dherman] 2011-04-08 10:21:19 PDT
Created attachment 524659 [details] [diff] [review]
final version for submission

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

Dave
Comment 20 Patrick Walton (:pcwalton) 2011-04-08 10:32:25 PDT
http://hg.mozilla.org/mozilla-central/rev/2ea06ff58dbe
Comment 21 George Carstoiu 2011-04-22 04:47:30 PDT
Verified on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110421 Firefox/6.0a1

Note You need to log in before you can comment on or make changes to this bug.