Last Comment Bug 614586 - Implement string substitution in all console API methods
: Implement string substitution in all console API methods
Status: RESOLVED FIXED
[parity-firebug/webkit] [console-1][f...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: P3 normal with 1 vote (vote)
: Firefox 9
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 696288
Blocks: 644596 685813 685815
  Show dependency treegraph
 
Reported: 2010-11-24 08:52 PST by David Dahl :ddahl
Modified: 2011-11-15 10:18 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Working patch (5.15 KB, patch)
2011-09-08 08:00 PDT, Panos Astithas [:past]
bugzilla: review+
bugs: review+
Details | Diff | Review
Working patch with extra tests (5.07 KB, patch)
2011-09-15 11:21 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
[in-fx-team] Working patch with extra tests and TODO comments (5.24 KB, patch)
2011-09-15 11:28 PDT, Panos Astithas [:past]
no flags Details | Diff | Review

Description David Dahl :ddahl 2010-11-24 08:52:35 PST
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 2010-12-01 06:49:04 PST
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?) 2011-01-06 18:40:22 PST
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 [:past] 2011-09-08 08:00:32 PDT
Created attachment 559157 [details] [diff] [review]
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 2011-09-08 11:10:47 PDT
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] (high review load, please consider other reviewers) 2011-09-14 09:37:39 PDT
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] (high review load, please consider other reviewers) 2011-09-14 09:49:34 PDT
Comment on attachment 559157 [details] [diff] [review]
Working patch

r+ if you add more tests.
Comment 7 Panos Astithas [:past] 2011-09-15 11:21:22 PDT
Created attachment 560416 [details] [diff] [review]
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 [:past] 2011-09-15 11:28:05 PDT
Created attachment 560417 [details] [diff] [review]
[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] 2011-09-16 02:26:25 PDT
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) 2011-09-21 04:49:49 PDT
https://hg.mozilla.org/mozilla-central/rev/8e3e1c5f348d
Comment 11 Eric Shepherd [:sheppy] 2011-11-15 10:18:13 PST
Docs updated:

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

Also mentioned on Firefox 9 for developers.

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