614586 - Implement string substitution in all console API methods

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[David Dahl :ddahl]

We will need a pre-processor for all input to the console API methods.

Following Firebug's lead here:

 %s 	String
 %d, %i Integer (numeric formatting is not yet supported)
 %f 	Floating point number (numeric formatting is not yet supported)
 %o 	Object hyperlink

Comment 1

[Kevin Dangoor]

This is relatively lower priority, since people will still at least see useful output without this improvement.

That said, both Firebug and Web Inspector do this, so if we sneak it in for Fx4 that would be a win.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I would ask to not tie this too tightly to the HUDService. I added ConsoleAPI support to Fennec, via the nsIConsoleService. But I can't really use HUDService directly.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

I have a working version here. I didn't implement width and precision qualifiers (e.g. %2.1f), but I intend to do it, probably in a followup. Firebug doesn't implement it either, but Chrome supports precision qualifiers at least.

I also treat object substitution (%o) the same as string substitution, in order to have something that works in Fennec as is. I thought about reusing the console.dir code in HUDService for this, but we'll have to come up with something similar for Fennec. I propose we do this, too, in a followup.

Finally, Firebug has grown another substitution pattern, %c, that changes the styling of the log messsage node, which is not supported by Chrome. I don't think it sounds useful enough to bother with getting all the edge cases right, not to mention avoiding any potential for misuse.

Comment 4

[David Dahl :ddahl]

Comment on attachment 559157 [details] [diff] [review]
Working patch

>diff --git a/dom/tests/browser/browser_ConsoleAPITests.js b/dom/tests/browser/browser_ConsoleAPITests.js
>--- a/dom/tests/browser/browser_ConsoleAPITests.js
>+++ b/dom/tests/browser/browser_ConsoleAPITests.js
>@@ -156,18 +156,23 @@ function expect(level) {
> function observeConsoleTest() {
>   let win = XPCNativeWrapper.unwrap(gWindow);
>   expect("log", "arg");
>   win.console.log("arg");
> 
>   expect("info", "arg", "extra arg");
>   win.console.info("arg", "extra arg");
> 
>-  expect("warn", "arg", "extra arg", 1);
>-  win.console.warn("arg", "extra arg", 1);
>+  // We don't currently support width and precision qualifiers, but we don't
>+  // choke on them either.
>+  expect("warn", "Lesson 1: PI is approximately equal to 3.14159");
>+  win.console.warn("Lesson %d: %s is approximately equal to %1.2f",
>+                   1,
>+                   "PI",
>+                   3.14159);

Nice patch. Very short and sweet. I built it and ran the tests, great! 

The only thing you should also do is file the followup bugs and add them to TODO comments in the code.

Since this code is in DOM you should probably get a dom peer to sign off as well.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

So what happens if you do something like
console.warn("%d, %s, %l");

What if you do console.warn("%a %b %c"); 

Please add some more tests.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 559157 [details] [diff] [review]
Working patch

r+ if you add more tests.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch with extra tests

(In reply to Olli Pettay [:smaug] from comment #5)
> So what happens if you do something like
> console.warn("%d, %s, %l");
> 
> What if you do console.warn("%a %b %c"); 
> 
> Please add some more tests.

Both of these strings are just displayed as is, since the substitution checks only take place with 2 or more arguments. This coincides with what the other browsers are doing.

I was having a hard time coming up with some meaningful and not entirely contrived corner cases to test against, but I've added tests for the ones you mentioned and a few more.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: [in-fx-team] Working patch with extra tests and TODO comments

Forgot to add the TODO comments Dave asked for.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 560417 [details] [diff] [review]
[in-fx-team] Working patch with extra tests and TODO comments

Pushed:
https://hg.mozilla.org/integration/fx-team/rev/8e3e1c5f348d

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/8e3e1c5f348d

Comment 11

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/Using_the_Web_Console#String_substitutions

Also mentioned on Firefox 9 for developers.