618344 - Web Console pprint() command should display something useful for functions

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[B.J. Herbison]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.10 (maverick) Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.10 (maverick) Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)

The Web Console pprint() command is supposed to format values, but when given a function it just displays blank lines.

Reproducible: Always

Steps to Reproduce:
1. Open the Web Console.
2. Type "pprint<enter>".
3. Type "pprint(pprint)<enter>".
Actual Results:  
The "pprint" displays the code of JSTH_pprint, but the "pprint(pprint)" just displays blank lines.

Expected Results:  
The "pprint(pprint)" command should display at least as much as the "pprint" command.

Comment 1

[B.J. Herbison]

The actual build id is:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101209 Firefox/4.0b8pre ID:20101209030340

Comment 2

[Mihai Sucan [:msucan]]

If you type "pprint<enter>" it's actually the output of the normal Web Console code which takes the input string, evaluates it (the result in this case is a function, the pprint function itself). The result is then displayed in the console output, as best as possible - that's function .toSource(). The pprint() function itself is not executed.

When you type "pprint(pprint)" you are again going through evaluation, but this time the pprint() function is executed, with the given argument (the pprint function itself). The pprint function tries to iterate the given argument, as an object, to output the properties the object has. The pprint() function itself has no properties, hence you see no output.

I believe the result is expected. This is not a bug.

If we'd hack the pprint() function to do toSource() when the argument is a function, then we'd no longer follow the initial purpose of the pprint() function.

Kevin: thoughts?

Comment 3

[David Dahl :ddahl]

(In reply to comment #2)

> If we'd hack the pprint() function to do toSource() when the argument is a
> function, then we'd no longer follow the initial purpose of the pprint()
> function.

There is noting wrong with that - just check to see if the function has any properties before using toSource()

Comment 4

[Kevin Dangoor]

Stepping back a bit, the original problem here is just that

pprint(someFunction)

prints a blank line, followed by the usual "undefined"

it's that blank line that's the problem. it should display *something* there.

Ultimately, pprint may be unnecessary. Firebug just uses console.log() and pretty prints objects and arrays and such automatically. That seems like a better approach.

Comment 5

[Kevin Dangoor]

To be clear: for this bug, we could simply display the function source.

If this is a trivial change, it may get into Firefox 4, otherwise this is likely for the next release.

Comment 6

[Mihai Sucan [:msucan]]

I tend to agree with Kevin. Bug 598357 made improvements to how we pretty print objects and the likes. We could keep pprint as it is, or remove it (we have the properties inspector panel).

Another possibility: instead of printing a blank line, we could say "no properties in the object". However, we have string freeze - so we can't do this.

Nonetheless, it is a trivial change. Will submit a patch which does toSource() for functions in pprint() - tomorrow.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Proposed fix.

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 498320 [details] [diff] [review]
proposed fix

Thanks Rob for the f+!

Asking for review from Shawn.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 498320 [details] [diff] [review]
proposed fix

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+    else if (typeof aObject === "function") {
magic string!  I'd like you to pull this out into a constant please (TYPEOF_FUNCTION maybe?).

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_jsterm.js
>  * Contributor(s):
>  *  David Dahl <[e-mail]>
>  *  Julian Viereck <[e-mail]>
>  *  Patrick Walton <[e-mail]>
>  *  Rob Campbell <[e-mail]>
>+ *  Mihai Èucan <[e-mail]>
note that the boilerplate asks for the names to be lined up with the "n" not the "o"

>+  // check that pprint(function) shows function source, bug 618344
>+  jsterm.clearOutput();
>+  jsterm.execute("pprint(print)");
>+  label = jsterm.outputNode.querySelector(".jsterm-output-line");
>+  isnot(label.textContent.indexOf("JSTH_print"), -1,
>+        "pprint(function) shows function source");
This doesn't look it's actually checking that the function source is being printed.  It should check that the output is, in fact, correct.

r=sdwilsh with that fixed.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch as requested. Thanks for the review+!

Asking for approval2.0+.

Comment 11

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

Comment on attachment 500199 [details] [diff] [review]
updated patch

I prefer the "magic string" over having to look up what TYPEOF_FUNCTION is defined as elsewhere in the file; typeof return values are fairly well known, so there isn't really any "magic". But I suppose it doesn't matter too much either way.

Comment 12

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

(In reply to comment #11)
> Comment on attachment 500199 [details] [diff] [review]
> updated patch
> 
> I prefer the "magic string" over having to look up what TYPEOF_FUNCTION is
> defined as elsewhere in the file; typeof return values are fairly well known,
> so there isn't really any "magic". But I suppose it doesn't matter too much
> either way.

ditto. I think that qualifies as an Übernit.

Comment 13

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/b8eed68150a3