Closed Bug 614586 Opened 14 years ago Closed 13 years ago

Implement string substitution in all console API methods

Categories

(DevTools :: General, defect, P3)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: ddahl, Assigned: past)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-firebug/webkit] [console-1][fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

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
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: devtools4
Priority: -- → P3
Whiteboard: [parity-firebug/webkit]
No longer blocks: devtools4
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.
Whiteboard: [parity-firebug/webkit] → [parity-firebug/webkit] [console-2]
Whiteboard: [parity-firebug/webkit] [console-2] → [parity-firebug/webkit] [console-1]
Blocks: 644596
Assignee: nobody → past
Status: NEW → ASSIGNED
Attached patch Working patch (obsolete) (deleted) — Splinter Review
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)
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+
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 on attachment 559157 [details] [diff] [review] Working patch r+ if you add more tests.
Attachment #559157 - Flags: review?(Olli.Pettay) → review+
Attached patch Working patch with extra tests (obsolete) (deleted) — Splinter Review
(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]
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
Whiteboard: [parity-firebug/webkit] [console-1][land-in-fx-team] → [parity-firebug/webkit] [console-1][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 696288
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: