Closed
Bug 614586
Opened 14 years ago
Closed 13 years ago
Implement string substitution in all console API methods
Categories
(DevTools :: General, defect, P3)
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)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 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.
Comment 2•14 years ago
|
||
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•14 years ago
|
Whiteboard: [parity-firebug/webkit] → [parity-firebug/webkit] [console-2]
Reporter | ||
Updated•14 years ago
|
Whiteboard: [parity-firebug/webkit] [console-2] → [parity-firebug/webkit] [console-1]
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•13 years ago
|
||
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•13 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+
Comment 5•13 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•13 years ago
|
||
Comment on attachment 559157 [details] [diff] [review]
Working patch
r+ if you add more tests.
Attachment #559157 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 7•13 years ago
|
||
(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
Assignee | ||
Updated•13 years ago
|
Whiteboard: [parity-firebug/webkit] [console-1] → [parity-firebug/webkit] [console-1][land-in-fx-team]
Assignee | ||
Comment 8•13 years ago
|
||
Forgot to add the TODO comments Dave asked for.
Attachment #560416 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
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•13 years ago
|
Whiteboard: [parity-firebug/webkit] [console-1][land-in-fx-team] → [parity-firebug/webkit] [console-1][fixed-in-fx-team]
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 10•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 11•13 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•