Implement string substitution in all console API methods

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ddahl, Assigned: past)

Tracking

({dev-doc-complete})

Trunk
Firefox 9
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-firebug/webkit] [console-1][fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
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.
Blocks: 593956
Priority: -- → P3
Whiteboard: [parity-firebug/webkit]

Updated

7 years ago
No longer blocks: 593956
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.
(Reporter)

Updated

6 years ago
Whiteboard: [parity-firebug/webkit] → [parity-firebug/webkit] [console-2]
(Reporter)

Updated

6 years ago
Whiteboard: [parity-firebug/webkit] [console-2] → [parity-firebug/webkit] [console-1]

Updated

6 years ago
Blocks: 644596
Assignee: nobody → past
Status: NEW → ASSIGNED
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.
Attachment #559157 - Flags: review?(ddahl)
(Reporter)

Comment 4

6 years ago
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.
Attachment #559157 - Flags: review?(ddahl)
Attachment #559157 - Flags: review?(Olli.Pettay)
Attachment #559157 - Flags: review+
Blocks: 685813
Blocks: 685815

Comment 5

6 years ago
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

6 years ago
Comment on attachment 559157 [details] [diff] [review]
Working patch

r+ if you add more tests.
Attachment #559157 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #559157 - Attachment is obsolete: true
Whiteboard: [parity-firebug/webkit] [console-1] → [parity-firebug/webkit] [console-1][land-in-fx-team]
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.
Attachment #560416 - Attachment is obsolete: true
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
Attachment #560417 - Attachment description: Working patch with extra tests and TODO comments → [in-fx-team] Working patch with extra tests and TODO comments

Updated

6 years ago
Whiteboard: [parity-firebug/webkit] [console-1][land-in-fx-team] → [parity-firebug/webkit] [console-1][fixed-in-fx-team]
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/8e3e1c5f348d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 696288
Docs updated:

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

Also mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.