598357 - use toSource() instead of toString() for some types of output in the Console

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

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

Currently, results of evaluated JS in the Web Console are being dumped using result.toString(). For some results, toSource() gives better output.

For strings, we should investigate the use of uneval().

from IRC:

09:25 <@jorendorff> [toSource is] not for objects beyond Object or Array
09:25 <@jorendorff> window.document.body  ===> [object HTMLBodyElement]           this is better 
                    than what toSource would do!
09:25 <@jorendorff> toSource would give you:      ({})
09:26 <@jorendorff> which is useless of course
09:26 <@jorendorff> so I guess toSource is better for strings, Objects, and Arrays
09:28 <@jorendorff> also objects of the types Date, Boolean, String, Number -- probably most if 
                    not all of the builtin types
09:28 <@jorendorff> but not DOM nodes.

Comment 1

[Jason Orendorff [:jorendorff]]

Date is dubious, actually. And: because toSource is wrong for DOM nodes, it's also wrong for arrays of DOM nodes.

But yeah, uneval is a whole lot better for strings!

 > "\n\n\n"
 "\n\n\n"            (better than 3 blank lines)
 > myobject.myprop
 "42"                (better than 42, which looks like a number)
 > "\u202ehello world"
 "\u202ehello world" (better than "dlrow olleh")

If you can uneval only strings, and it's a two-line hack, that might be worth doing by itself.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Please note that:

- bug 586249 has pretty much the same purpose as this bug report, but it proposes a slightly different approach to presenting the jsterm output, because it prefers to format the output a bit more. In this case we just use toSource() when we deem it necessary. (I prefer this approach.)

- bug 595195 touches only the output of strings, to not make them links/inspectable.

The two patches above conflict with this one.

The patch I am attaching here does the following:

- "formats" the output better by using toSource() in some cases. Btw, as I see there's no need to check if an object is instanceof Node or such, because we can just check if toSource() returns "({})", this way we catch more objects that don't have nice toSource() output (like window.location).

- makes numbers, booleans, strings, regexp and date objects non-inspectable. Currently if the user clicks them, he/she sees an empty property panel or at best an array of chars for strings. (not useful)

- uses the same formatting code for the window.console API. This is something bug 586249 also aimed for.

- the test code is the same as in bug 595195 but it's now more generic in its purpose.

I suggest marking bug 595195 as a duplicate of this bug, and canceling the review request. Not sure what we should do with bug 586249.

Thanks!

Comment 3

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

Comment on attachment 478003 [details] [diff] [review]
proposed fix and test code

-    // TODO: format the aOutputObject and don't just use the
-    // aOuputObject.toString() function: [object object] -> Object {prop, ...}
-    // See bug 586249.
-    let textNode = this.textFactory(aOutputObject + "\n");
+    let textNode = this.textFactory(aOutputString + "\n");

Will this fix bug 586249 as well?

...

+    let type = aResult && aResult.constructor ?
+      aResult.constructor.name : "";
+    if (!type) {
+      type = aResult === null ? "null" : typeof aResult;
+    }

cute.

You repeat this block in resultInspectable. Maybe break it into a separate method called resultType() or similar?

+    switch (type.toLowerCase()) {
+      case "string":
+        if (!isNaN(aResult)) {
+          output = '"' + aResult + '"';
+        }
+        else {

I'd prefer to the see this as if (isNaN(aResult)) ... Testing for the negative is more confusing, imo.

and from the test code...

+let inputValues = [
+  // [showsPropertyPanel?, input value, expected output format]
+  [false, "'hello from the string world!'", "hello from the string world!"],
+  [false, "'úṇĩçödê țĕșť'", "úṇĩçödê țĕșť"],
...

covered all the types, it looks like.

+    if (popupShown.length == inputValues.length) {
+      executeSoon(testEnd);

is that executeSoon needed?

+  // Ugly but it does the job.
+  with (content) {
+    eval("HUD.console.log(" + inputValue.
+      replace("document.", "document.wrappedJSObject.") + ")");
+  }

wow! (that's it, no comment)

This looks like it'll do the job, though it's a surprising amount of change for slightly-better formatting. I guess a good chunk of that is test code.

f+ with the above minor changes.

Comment 4

[Mihai Sucan [:msucan]]

(In reply to comment #3)
> Comment on attachment 478003 [details] [diff] [review]
> proposed fix and test code
> 
> -    // TODO: format the aOutputObject and don't just use the
> -    // aOuputObject.toString() function: [object object] -> Object {prop, ...}
> -    // See bug 586249.
> -    let textNode = this.textFactory(aOutputObject + "\n");
> +    let textNode = this.textFactory(aOutputString + "\n");
> 
> Will this fix bug 586249 as well?

Yes. Julian made an even bigger patch that does more custom formatting, rather than using toSource(). For some reason, and for delays caused by bug 583476 his patch didn't get any feedback+ / review+.

I found his report working on this bug.

> 
> +    let type = aResult && aResult.constructor ?
> +      aResult.constructor.name : "";
> +    if (!type) {
> +      type = aResult === null ? "null" : typeof aResult;
> +    }
> 
> cute.
> 
> You repeat this block in resultInspectable. Maybe break it into a separate
> method called resultType() or similar?

Sure.

> +    switch (type.toLowerCase()) {
> +      case "string":
> +        if (!isNaN(aResult)) {
> +          output = '"' + aResult + '"';
> +        }
> +        else {
> 
> I'd prefer to the see this as if (isNaN(aResult)) ... Testing for the negative
> is more confusing, imo.

Fixed.


> and from the test code...
> 
> +let inputValues = [
> +  // [showsPropertyPanel?, input value, expected output format]
> +  [false, "'hello from the string world!'", "hello from the string world!"],
> +  [false, "'úṇĩçödê țĕșť'", "úṇĩçödê țĕșť"],
> ...
> 
> covered all the types, it looks like.
> 
> +    if (popupShown.length == inputValues.length) {
> +      executeSoon(testEnd);
> 
> is that executeSoon needed?

I use executeSoon() when I need a bit of delay, to allow the main code to "complete" execution.


> +  // Ugly but it does the job.
> +  with (content) {
> +    eval("HUD.console.log(" + inputValue.
> +      replace("document.", "document.wrappedJSObject.") + ")");
> +  }
> 
> wow! (that's it, no comment)

:)


> This looks like it'll do the job, though it's a surprising amount of change for
> slightly-better formatting. I guess a good chunk of that is test code.
> 
> f+ with the above minor changes.

Thanks for your feedback+! Will attach the updated patch.

The amount of change is surprising because I had to "modularize" (formatResult, resultInspectable, resultType methods) the code, so I can reuse it from the HUDConsole code which outputs window.console messages. Before making this change, the code was packed into writeOutput() and writeOutputJS() in a more minimalistic approach. And of course... the test code tries to cover a lot of ground.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch with the changes requested by Robert.

Comment 9

[Kevin Dangoor]

Requesting blocking status for this bug. This bug now contains the fix for several instances of strange appearance or behavior for the results of JavaScript expressions.

Comment 10

[Jason Orendorff [:jorendorff]]

>+        if (isNaN(aResult)) {
>+          output = aResult;
>+        }
>+        else {
>+          output = '"' + aResult + '"';
>+        }

It seems like this will only improve cases where aResult looks like a number or is all whitespace. The results seem kind of random:


  aResult           output
  -------           ------

  ""                ""
  "true"            true
  true              true
  "null"            null
  null              null
  "[]"              []
  "inf"             "inf"
  "ing"             ing
  "\n\n"            "

                    "
Why not just always show the quotes?

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> >+        if (isNaN(aResult)) {
> >+          output = aResult;
> >+        }
> >+        else {
> >+          output = '"' + aResult + '"';
> >+        }
> 
> It seems like this will only improve cases where aResult looks like a number or
> is all whitespace. The results seem kind of random:
> 
> 
>   aResult           output
>   -------           ------
> 
>   ""                ""
>   "true"            true
>   true              true
>   "null"            null
>   null              null
>   "[]"              []
>   "inf"             "inf"
>   "ing"             ing
>   "\n\n"            "
> 
>                     "
> Why not just always show the quotes?

I know what you mean, and I was thorn about this. If I put quotes around strings, then for a string that includes quotes we see something like:

"foo"bar""

Which is ... not nice. Firebug does this, IIANM. One way to work around this is to use uneval():

"foo\"bar\""

That's good. But when you have a string with unicode characters, like "Mihai Șucan", you get:

"Mihai \u0218ucan"

... with which I cannot agree. There's simply to much content out there with unicode chars, and displaying it like this is really ugly and not user-friendly. We do have, as I understand, a much wider target audience for these developer tools, than say Firebug. I'd like us to keep strings readable.

The compromise was to show quotes around numbers that are strings, to make it obvious for the *human* who uses the Web Console.

If Kevin/Robert/David would like to chime in and make the final decision on the matter, sure, please do! Ultimately, I do not mind if a different decision is reached. I'll update the patch accordingly.

Thanks for your comment!

Comment 12

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

Readability's a noble goal, to be sure. But I'd also like to be able to distinguish between strings and booleans.

I agree with you, Mihai, uneval produces some ugly results for unicode characters, but I like the escaped quotes.

One solution might be to detect (double) quotes inside a string and replace them with escaped quotes, i.e., \". It's more logic, but it would help the display.

What do you think, Jason?

Comment 13

[Kevin Dangoor]

(Re: previous comment: don't forget single quotes as well!)

For comparison's sake, Python's REPL displays the uneval equivalent.

>>> "\n\n"
'\n\n'


You can use print if you want to see the string in its final representation:
>>> print "\n\n"



>>> 

If we went with that route, you could use console.log() to display the string value.

I think it's a question of which case we optimize for: display of a potentially long string with newlines and such in a readable way or display in a form that is convenient for unambiguous copy/paste.

RobC's suggestion strikes a middle ground of just escaping quotes... but that does potentially produce a string that's invalid on copy/paste if it contains newlines.

My hunch (and it's nothing more than a hunch from having used REPLs a lot) is that displaying the value in a nice-to-read format solves the more common case of just wanting to visually inspect the value. I do think it's important for the user to be able to use uneval or something to be able to inspect the string and know for sure which exact codepoints are in it.

Comment 14

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

Fair points, Kevin. And I think I've already created a user who likes to copy and paste text from the console as one of the main consumers of this console so it's good to keep them in mind.

What about taking the quote-escaping to the next level and replace quotes and whitespace with their escaped equivalents?

\t, \n, \r, \",... any others?

So we can avoid seeing Mihai's name with escaped unicode. Personally, I think that would look best, but might be getting a little ugly in code.

Comment 15

[Mihai Sucan [:msucan]]

Attachment: Patch update 2

Updated patch based on the above comments. I also discussed with Kevin to reach the final decision on how we want things to work. Patch changes:

- added a new print() jsterm helper function, which displays objects as strings, without any formatting (toString()).

- fixed a problem which caused jsterm helper functions to always show undefined after execution.

- made it such that jsterm helper functions have precedence in evaluation over globals. For example, print() did not work (window.print had precedence).

- changed string output to be a mix between uneval() and string output as-is (per Kevin's decision). We take the string through uneval() then we unescape human-readable characters (\u-escaped characters and a small range or \x-escaped characters). This makes unicode strings like "Mihai Șucan" display as expected, while showing "\tnew\nlines" nicely.

- updated mochitests to properly test the changed functionality.


Asking for feedback once again, due to the number of changes. Thanks!

Comment 16

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

Comment on attachment 481871 [details] [diff] [review]
Patch update 2

     clear: function JSTH_clear()
     {
+      aJSTerm.helperEvaluated_ = true;

what's with the dangling_?

   evalInSandbox: function JST_evalInSandbox(aString)
   {
-    let execStr = "with(__helperFunctions__) { with(window) {" + aString + "} }";
+    let execStr = "with(window) { with(__helperFunctions__) { " + aString + " } }";

changed this in bug 599940. Should not have to change this line anymore.

+  /**
+   * Format a string to for output.
+   *

Lose the "to"?

+
+    let output = uneval(aString).replace(/\\(x)([0-9a-fA-F]{2})/g, replaceFn).
+      replace(/\\(u)([0-9a-fA-F]{4})/g, replaceFn);

chained function should have the . preceding the second line. Opinions may vary on this.

...

 	browser_webconsole_bug_588342_document_focus.js \
+	browser_webconsole_bug_598357_jsterm_output.js \
 	$(NULL)

already added back in. You'll need to make sure your unittests here still work and update the patch to merge.

This has turned into a pretty big, dense piece of code for what was ostensibly a simple request. I'm not sure we should be doing something this complicated for this, or if we could do it in smaller, more digestible chunks like bug 608564.

Nevertheless, this looks like it should work at the expense of changing some of our existing APIs.

Comment 18

[Mihai Sucan [:msucan]]

(In reply to comment #16)
> Comment on attachment 481871 [details] [diff] [review]
> Patch update 2
> 
>      clear: function JSTH_clear()
>      {
> +      aJSTerm.helperEvaluated_ = true;
> 
> what's with the dangling_?
> 
>    evalInSandbox: function JST_evalInSandbox(aString)
>    {
> -    let execStr = "with(__helperFunctions__) { with(window) {" + aString + "}
> }";
> +    let execStr = "with(window) { with(__helperFunctions__) { " + aString + "
> } }";
> 
> changed this in bug 599940. Should not have to change this line anymore.
> 
> +  /**
> +   * Format a string to for output.
> +   *
> 
> Lose the "to"?
> 
> +
> +    let output = uneval(aString).replace(/\\(x)([0-9a-fA-F]{2})/g, replaceFn).
> +      replace(/\\(u)([0-9a-fA-F]{4})/g, replaceFn);
> 
> chained function should have the . preceding the second line. Opinions may vary
> on this.
> 
> ...
> 
>      browser_webconsole_bug_588342_document_focus.js \
> +    browser_webconsole_bug_598357_jsterm_output.js \
>      $(NULL)
> 
> already added back in. You'll need to make sure your unittests here still work
> and update the patch to merge.

I'll attach the updated patch, rebased and fixed to work with the latest HUDService changes. I'll also include the changes you requested. Thanks for the feedback+!


> This has turned into a pretty big, dense piece of code for what was ostensibly
> a simple request. I'm not sure we should be doing something this complicated
> for this, or if we could do it in smaller, more digestible chunks like bug
> 608564.
> 
> Nevertheless, this looks like it should work at the expense of changing some of
> our existing APIs.

It turned out big because we wanted more than was initially requested. Still, the changes are pretty simple, I'd say.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Patch update 3, based on feedback from Robert.

Asking for review now.

Comment 20

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

Comment on attachment 487539 [details] [diff] [review]
patch update 3

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>     try {
>+      this.helperEvaluated = false;
>       var result = this.evalInSandbox(aExecuteString);
> 
>+      if (result !== undefined || !this.helperEvaluated) {

This doesn't look right - !helperEvaluated will always be true, given that it's set to false just above.

>+  formatString: function JST_formatString(aString)

>+      // Printable characters.
>+      if ((decimal >= 33 && decimal <= 126) || decimal >= 161) {
>+        result = String.fromCharCode(decimal);
>+      }
>+      else {
>+        result = "\\" + aType + aHex;
>+      }

This is rather hacky... I wish there were a better way to do this, but I can't really think of any. We should at least document what the 33/126/161 magic numbers represent.

>+  resultType: function JST_resultType(aResult)
>+  {
>+    let type = aResult && aResult.constructor ?
>+      aResult.constructor.name : "";
>+    if (!type) {
>+      type = aResult === null ? "null" : typeof aResult;

This method can be fooled by tricks like (new (function boolean(){})).constructor.name. I suppose that probably isn't a big deal, but perhaps you should prefer typeof results over constructor.name?

Comment 21

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

(In reply to comment #20)
> Comment on attachment 487539 [details] [diff] [review]
> >+      // Printable characters.
> >+      if ((decimal >= 33 && decimal <= 126) || decimal >= 161) {
> >+        result = String.fromCharCode(decimal);
> >+      }
> >+      else {
> >+        result = "\\" + aType + aHex;
> >+      }
> 
> This is rather hacky... I wish there were a better way to do this, but I can't
> really think of any. We should at least document what the 33/126/161 magic
> numbers represent.

I had a problem with this too when reading it.

Maybe define some constants describing what the numbers are, i.e., string.charCodeAt?

> >+  resultType: function JST_resultType(aResult)
> >+  {
> >+    let type = aResult && aResult.constructor ?
> >+      aResult.constructor.name : "";
> >+    if (!type) {
> >+      type = aResult === null ? "null" : typeof aResult;
> 
> This method can be fooled by tricks like (new (function
> boolean(){})).constructor.name. I suppose that probably isn't a big deal, but
> perhaps you should prefer typeof results over constructor.name?

we've been using the constructor.name hack in a few^W places^H. I think largely because typeof isn't accurate for some types.

Can only find it in action here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/PropertyPanel.jsm#76

Comment 22

[Mihai Sucan [:msucan]]

(In reply to comment #20)
> Comment on attachment 487539 [details] [diff] [review]
> patch update 3
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >     try {
> >+      this.helperEvaluated = false;
> >       var result = this.evalInSandbox(aExecuteString);
> > 
> >+      if (result !== undefined || !this.helperEvaluated) {
> 
> This doesn't look right - !helperEvaluated will always be true, given that it's
> set to false just above.

Not always. It is an unfortunate hack, to not always print "undefined" when a jsterm helper is evaluated. Currently we have jsterm helpers which do not return any value and we show "undefined" which is weird/broken.

Look into the patch: helperEvaluated is set to true at times.

If you have better ideas on how to fix this issue, please let me know.


> >+  formatString: function JST_formatString(aString)
> 
> >+      // Printable characters.
> >+      if ((decimal >= 33 && decimal <= 126) || decimal >= 161) {
> >+        result = String.fromCharCode(decimal);
> >+      }
> >+      else {
> >+        result = "\\" + aType + aHex;
> >+      }
> 
> This is rather hacky... I wish there were a better way to do this, but I can't
> really think of any. We should at least document what the 33/126/161 magic
> numbers represent.

The comment was intended for that: printable characters. Not sure, what shall I add? Shall I use constants as suggested by Robert?

> >+  resultType: function JST_resultType(aResult)
> >+  {
> >+    let type = aResult && aResult.constructor ?
> >+      aResult.constructor.name : "";
> >+    if (!type) {
> >+      type = aResult === null ? "null" : typeof aResult;
> 
> This method can be fooled by tricks like (new (function
> boolean(){})).constructor.name. I suppose that probably isn't a big deal, but
> perhaps you should prefer typeof results over constructor.name?

I used typeof initially, and it did not work as good as constructor.name. Do you want me to change to typeof? I am certain tests will fail if I do this.

Thanks for your review!

Comment 23

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

(In reply to comment #22)
> (In reply to comment #20)
> > Comment on attachment 487539 [details] [diff] [review] [details]
> > patch update 3
> > 
> > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> > 
> > >     try {
> > >+      this.helperEvaluated = false;
> > >       var result = this.evalInSandbox(aExecuteString);
> > > 
> > >+      if (result !== undefined || !this.helperEvaluated) {
> > 
> > This doesn't look right - !helperEvaluated will always be true, given that it's
> > set to false just above.
> 
> Not always. It is an unfortunate hack, to not always print "undefined" when a
> jsterm helper is evaluated. Currently we have jsterm helpers which do not
> return any value and we show "undefined" which is weird/broken.
> 
> Look into the patch: helperEvaluated is set to true at times.
> 
> If you have better ideas on how to fix this issue, please let me know.

I do not.

> > >+  formatString: function JST_formatString(aString)
> > 
> > >+      // Printable characters.
> > >+      if ((decimal >= 33 && decimal <= 126) || decimal >= 161) {
> > >+        result = String.fromCharCode(decimal);
> > >+      }
> > >+      else {
> > >+        result = "\\" + aType + aHex;
> > >+      }
> > 
> > This is rather hacky... I wish there were a better way to do this, but I can't
> > really think of any. We should at least document what the 33/126/161 magic
> > numbers represent.
> 
> The comment was intended for that: printable characters. Not sure, what shall I
> add? Shall I use constants as suggested by Robert?

Yes, I think that would improve readability.

> > >+  resultType: function JST_resultType(aResult)
> > >+  {
> > >+    let type = aResult && aResult.constructor ?
> > >+      aResult.constructor.name : "";
> > >+    if (!type) {
> > >+      type = aResult === null ? "null" : typeof aResult;
> > 
> > This method can be fooled by tricks like (new (function
> > boolean(){})).constructor.name. I suppose that probably isn't a big deal, but
> > perhaps you should prefer typeof results over constructor.name?
> 
> I used typeof initially, and it did not work as good as constructor.name. Do
> you want me to change to typeof? I am certain tests will fail if I do this.

Maybe if you reversed the ordering of your type declaration this would satisfy our weary reviewer.

resultType: function JST_resultType(aResult)
{
  if (aResult === null)
    return "null";

  let type = typeof aResult;
  if (type == "object")
    type = aResult.constructor.name;

  return type;
}


what do you think, gavin?

Comment 24

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

(In reply to comment #22)
> Not always. It is an unfortunate hack, to not always print "undefined" when a
> jsterm helper is evaluated. Currently we have jsterm helpers which do not
> return any value and we show "undefined" which is weird/broken.

I see, I just misread the code (it wasn't obvious that the call to evalInSandbox could affect helperEvaluated's value). Can the helpers just return their output, rather than printing them directly?

It seems like the patch's approach will result in expressions that happen to include a helper not producing output, e.g. evaluating |print("hi"), typeof("foo");| won't print out "string" as it should.

https://www.squarefree.com/shell/ seems to handle these cases correctly so it may be worth investigating how it does it.

> The comment was intended for that: printable characters. Not sure, what shall I
> add? Shall I use constants as suggested by Robert?

All I'm saying is that "printable characters" is not specific enough. You're only including printable ASCII characters, it looks like, which is not the only definition of "printable characters" in the realm of unicode - just think you should clarify that.

(In reply to comment #23)
> Maybe if you reversed the ordering of your type declaration

Yes, this is what I was trying to suggest.

Comment 25

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 26

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 27

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 28

[Mihai Sucan [:msucan]]

(In reply to comment #24)
> (In reply to comment #22)
> > Not always. It is an unfortunate hack, to not always print "undefined" when a
> > jsterm helper is evaluated. Currently we have jsterm helpers which do not
> > return any value and we show "undefined" which is weird/broken.
> 
> I see, I just misread the code (it wasn't obvious that the call to
> evalInSandbox could affect helperEvaluated's value). Can the helpers just
> return their output, rather than printing them directly?

Thought of doing that, but each helper may print its output as needed, without any further formating done by the main execute() method. pprint() does its own output formatting - if it goes through the normal path, it produces undesired results. Similarly, if print()'s output goes through the string formatting code of execute() ... it produces an undesired output.


> It seems like the patch's approach will result in expressions that happen to
> include a helper not producing output, e.g. evaluating |print("hi"),
> typeof("foo");| won't print out "string" as it should.

That's not the case. Only if that expression results in undefined output, then the result is not displayed. The example expression will print "string".


> > The comment was intended for that: printable characters. Not sure, what shall I
> > add? Shall I use constants as suggested by Robert?
> 
> All I'm saying is that "printable characters" is not specific enough. You're
> only including printable ASCII characters, it looks like, which is not the only
> definition of "printable characters" in the realm of unicode - just think you
> should clarify that.

Will try this in the updated patch.

> (In reply to comment #23)
> > Maybe if you reversed the ordering of your type declaration
> 
> Yes, this is what I was trying to suggest.

Will do.

Thanks for your reply!

Comment 29

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated patch based on review.

Comment 30

[Mihai Sucan [:msucan]]

Comment on attachment 493019 [details] [diff] [review]
patch update 4

Asking for review from Dietrich.

Comment 31

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

Comment on attachment 493019 [details] [diff] [review]
patch update 4

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+    let mappedArguments = Array.prototype.map.call(aArguments,
>+        hud.jsterm.formatResult, hud.jsterm);

Array.map(aArguments, hud.jsterm.formatResult, hud.jsterm); is more concise.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  formatString: function JST_formatString(aString)

>+    let replaceFn = function(aMatch, aType, aHex) {

Prefer |function replaceFn()|

>+      // Show printable characters from the ASCII table, and all of the Unicode
>+      // printable characters (considered to start from 
>+      // CHAR_EXCLAMATION_INVERTED).

This is still rather confusing. It looks to me like this code excludes the C0 and C1 control codes (http://en.wikipedia.org/wiki/C0_and_C1_control_codes) specifically, in addition to space (U+0020), Delete (U+007F), and Non-breaking-space (U+00A0), which isn't quite the inverse of "Unicode printable characters" (a set that AFAICT isn't very well defined, particularly since "printability" in the general sense can end up largely depending on system fonts). Space isn't a big deal since uneval doesn't escape it, and the other two characters make sense to treat as "unprintable" (former actually is, the latter doesn't want to be confused with a normal space), but I would prefer something more like:

function replaceFn(aMatch, aType, aHex) {
  function isControlCode(c) {
    // See http://en.wikipedia.org/wiki/C0_and_C1_control_codes
    // C0 is 0x00-0x1F, C1 is 0x80-0x9F (inclusive)
    // We also include DEL (U+007F) and NBSP (U+00A0), which are not strictly
    // in C1 but border it.
    return (c <= 0x1F) ||
           (0x7F <= c && c <= 0xA0);
  }

  let c = parseInt(aHex, 16);
  if (isControlCode(c))
    return aMatch; // leave it escaped
    // "\\" + aType + aHex; 

  return String.fromCharCode(c); // unescape
}

>+  resultInspectable: function JST_resultInspectable(aResult)

I wonder if this wouldn't be more simply implemented using something like:

var isEnumerable = false;
for (var p in aResult) {
  isEnumerable = true;
  break;
}
return isEnumerable && typeof(aResult) != "string";

There are probably some edge cases that doesn't catch... as with every part of this patch! This is somewhat frustrating problem to solve - JavaScript's idiosyncrasies makes any workable solution seem very hacky :) The test coverage here is good and we can extend it as we discover other cases that need to be handled differently, so I'll refrain from trying to rethink this too much.

Comment 32

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

thanks for the review Gavin. I'm more interested in landing something that works reasonably well and fixes some of the more egregious problems with output than something that fixes every edge case imaginable.

Comment 33

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Thank you Gavin for your review+!

I have rebased the patch and included the changes you requested (both for replaceFn and resultInspectable - they work fine). Additionally, I renamed resultInspectable() and resultType() to isResultInspectable() and getResultType(), as requested by Dietrich in an IRC discussion.

This is ready to land!

Comment 34

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

Comment on attachment 496789 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/6d62c9b35eb2