Closed
Bug 646025
Opened 14 years ago
Closed 14 years ago
Add file location to console.log, info, debug, etc.
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 6
People
(Reporter: rcampbell, Assigned: past)
Details
(Whiteboard: [console][fixed-in-devtools][merged-to-mozilla-central])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
In all of the console logging methods (console API, log, info, warn, error and now debug), the originating location of the log method should be displayed with the output in the console.
E.g., from a file test.js, on line 5:
console.log("this is some console output");
produces:
"this is some console output" test.js:5
Reporter | ||
Updated•14 years ago
|
Whiteboard: [console]
Updated•14 years ago
|
Assignee: nobody → past
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #529445 -
Flags: feedback?(mihai.sucan)
Comment 2•14 years ago
|
||
Comment on attachment 529445 [details] [diff] [review]
First patch attempt
Review of attachment 529445 [details] [diff] [review]:
The patch looks fine, but it needs some improvements. Please see the detailed feedback.
Additionally:
- if I run the ConsoleAPI tests they fail. You need to run the tests from dom/tests/browser as well (mochitest-browser-chrome).
- you need to update the ConsoleAPI tests from dom/tests/browser as well, not just the HUDService tests, to reflect the API changes.
- make sure you also run the tests from dom/tests/mochitest/general, so they don't regress. There's a ConsoleAPI test there. These tests are mochitest-plain. So your in your make command replace "mochitest-browser-chrome" with "mochitest-plain".
Thanks for your patch. Looking forward to your updated patch!
::: dom/base/ConsoleAPI.js
@@ +172,5 @@
+ if (!frame.caller) break;
+ frame = frame.caller;
+ }
+ args.filename = frame.filename;
+ args.lineNumber = frame.lineNumber;
I think we want to also include the function name, since it's only a matter of UI to add that bit of information as well, some time later.
Also, I think we want to reuse getStackTrace() or change the code somehow so we deal with the stackframes in a common way for console.trace() and the other methods.
::: toolkit/components/console/hudservice/HUDService.jsm
@@ +1999,5 @@
case "info":
case "warn":
case "error":
case "debug":
+ let mappedArguments = Array.map(aArguments.arguments, formatResult);
I am not sure if it's really nice to have arguments.arguments. Same applies to ConsoleAPI.js.
::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_646025_console_file_location.js
@@ +1,3 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
According to the latest guidelines we need to use the public domain license for tests. Please see:
http://www.mozilla.org/MPL/boilerplate-1.1/
http://www.mozilla.org/MPL/license-policy.html
@@ +44,5 @@
+
+const TEST_FILE_LOCATION_URI = "http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-bug-646025-console-file-location.html" + "?_date=" + Date.now();
+
+function test() {
+ dump("WHEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE\n");
This doesn't need to show up when we submit the patch for push to m-c/devtools, hehe. :)
@@ +52,5 @@
+
+function onLoad(aEvent) {
+ browser.removeEventListener(aEvent.type, arguments.callee, true);
+ openConsole();
+ hudId = HUDService.displaysIndex()[0];
This is deprecated API. Please use HUDService.getHudIdByWindow(content).
::: toolkit/components/console/hudservice/tests/browser/test-bug-646025-console-file-location.html
@@ +1,5 @@
+<!DOCTYPE HTML>
+<html dir="ltr" xml:lang="en-US" lang="en-US"><head>
+ <title>Console file location test</title>
+ <script src="test-file-location.js"></script>
+ </head>
Please add the PD license boilerplate in the header.
::: toolkit/components/console/hudservice/tests/browser/test-file-location.js
@@ +1,1 @@
+console.log("message for level log");
Please add the PD license boilerplate in the header.
Attachment #529445 -
Flags: feedback?(mihai.sucan) → feedback-
Assignee | ||
Comment 3•14 years ago
|
||
(In reply to comment #2)
> The patch looks fine, but it needs some improvements. Please see the detailed
> feedback.
Thanks! Just a small question while I'm getting ready to submit a followup patch:
> toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_646025_console_file_location.js
> @@ +1,3 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>
> According to the latest guidelines we need to use the public domain license for
> tests. Please see:
>
> http://www.mozilla.org/MPL/boilerplate-1.1/
> http://www.mozilla.org/MPL/license-policy.html
Since I copied this file from browser_webconsole_basic_net_logging.js, is there any problem with changing the license? I assume I would still have to retain the list of contributors? Do I have to contact them for license change approval?
Comment 4•14 years ago
|
||
(In reply to comment #3)
> Since I copied this file from browser_webconsole_basic_net_logging.js, is there
> any problem with changing the license? I assume I would still have to retain
> the list of contributors? Do I have to contact them for license change
> approval?
I think you don't have to bother with that. You can just go ahead and switch the license.
Assignee | ||
Comment 5•14 years ago
|
||
Updated patch after incorporating feedback.
Attachment #529445 -
Attachment is obsolete: true
Attachment #529672 -
Flags: feedback?(mihai.sucan)
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #2)
> Comment on attachment 529445 [details] [diff] [review] [review]
> First patch attempt
>
> Review of attachment 529445 [details] [diff] [review] [review]:
>
> The patch looks fine, but it needs some improvements. Please see the detailed
> feedback.
>
> Additionally:
>
> - if I run the ConsoleAPI tests they fail. You need to run the tests from
> dom/tests/browser as well (mochitest-browser-chrome).
> - you need to update the ConsoleAPI tests from dom/tests/browser as well, not
> just the HUDService tests, to reflect the API changes.
> - make sure you also run the tests from dom/tests/mochitest/general, so they
> don't regress. There's a ConsoleAPI test there. These tests are
> mochitest-plain. So your in your make command replace
> "mochitest-browser-chrome" with "mochitest-plain".
All of the following tests pass now:
TEST_PATH=toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_646025_console_file_location.js make -C obj-ff-dbg mochitest-browser-chrome
TEST_PATH=dom/tests/browser make -C obj-ff-dbg mochitest-browser-chrome
TEST_PATH=dom/tests/mochitest/general/ make -C obj-ff-dbg mochitest-plain
> Thanks for your patch. Looking forward to your updated patch!
>
> ::: dom/base/ConsoleAPI.js
> @@ +172,5 @@
> + if (!frame.caller) break;
> + frame = frame.caller;
> + }
> + args.filename = frame.filename;
> + args.lineNumber = frame.lineNumber;
>
> I think we want to also include the function name, since it's only a matter of
> UI to add that bit of information as well, some time later.
Done.
> Also, I think we want to reuse getStackTrace() or change the code somehow so we
> deal with the stackframes in a common way for console.trace() and the other
> methods.
I'm calling getStackTrace() now. My main concern was that it would cause extra work to be done while creating the full stack trace, since we only need the first four frames in most cases, but it's probably not worth splitting getStackTrace() in two.
> ::: toolkit/components/console/hudservice/HUDService.jsm
> @@ +1999,5 @@
> case "info":
> case "warn":
> case "error":
> case "debug":
> + let mappedArguments = Array.map(aArguments.arguments, formatResult);
>
> I am not sure if it's really nice to have arguments.arguments. Same applies to
> ConsoleAPI.js.
Agreed, renamed arguments to params.
> :::
> toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_646025_console_file_location.js
> @@ +1,3 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>
> According to the latest guidelines we need to use the public domain license for
> tests. Please see:
>
> http://www.mozilla.org/MPL/boilerplate-1.1/
> http://www.mozilla.org/MPL/license-policy.html
Done.
> @@ +44,5 @@
> +
> +const TEST_FILE_LOCATION_URI =
> "http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-bug-646025-console-file-location.html"
> + "?_date=" + Date.now();
> +
> +function test() {
> + dump("WHEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE\n");
>
> This doesn't need to show up when we submit the patch for push to m-c/devtools,
> hehe. :)
Heh, oops!
> @@ +52,5 @@
> +
> +function onLoad(aEvent) {
> + browser.removeEventListener(aEvent.type, arguments.callee, true);
> + openConsole();
> + hudId = HUDService.displaysIndex()[0];
>
> This is deprecated API. Please use HUDService.getHudIdByWindow(content).
Done.
> :::
> toolkit/components/console/hudservice/tests/browser/test-bug-646025-console-file-location.html
> @@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US"><head>
> + <title>Console file location test</title>
> + <script src="test-file-location.js"></script>
> + </head>
>
> Please add the PD license boilerplate in the header.
Done.
> ::: toolkit/components/console/hudservice/tests/browser/test-file-location.js
> @@ +1,1 @@
> +console.log("message for level log");
>
> Please add the PD license boilerplate in the header.
Done.
Thanks for the thorough feedback!
Comment 7•14 years ago
|
||
(In reply to comment #3)
> Since I copied this file from browser_webconsole_basic_net_logging.js, is there
> any problem with changing the license? I assume I would still have to retain
> the list of contributors? Do I have to contact them for license change
> approval?
You should probably check with gerv just to be safe.
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 529672 [details] [diff] [review]
Updated patch
Asking gerv for feedback too, then.
gerv: See comment 3 above.
Attachment #529672 -
Flags: feedback?(gerv)
Comment 9•14 years ago
|
||
(In reply to comment #3)
> Since I copied this file from browser_webconsole_basic_net_logging.js, is there
> any problem with changing the license? I assume I would still have to retain
> the list of contributors? Do I have to contact them for license change
> approval?
If you copy code out of a tri-licensed file, you need to make the result tri-licensed. You can use Public Domain for tests if you want to (it's not a requirement), but only if they are written from scratch.
Let me know if that doesn't answer your question :-)
Gerv
Assignee | ||
Comment 10•14 years ago
|
||
Nope, you've been crystal clear :-)
Assignee | ||
Comment 11•14 years ago
|
||
Restored the initial license text for the test file per gerv's comment and rebased after the last merge from m-c.
Attachment #529672 -
Attachment is obsolete: true
Attachment #529672 -
Flags: feedback?(mihai.sucan)
Attachment #529672 -
Flags: feedback?(gerv)
Attachment #529726 -
Flags: feedback?(mihai.sucan)
Comment 12•14 years ago
|
||
Comment on attachment 529726 [details] [diff] [review]
Third version of the patch
Review of attachment 529726 [details] [diff] [review]:
The patch looks good. Thank you for the update!
I think we need to add a location check in the Console API tests as well. You only check if the location is displayed in the new HUDService test. Look into how the stack trace test works in the ConsoleAPI tests code. Perhaps you can do something similar (or better) for console.log() as well.
Giving the patch f- only for the missing test.
::: toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_646025_console_file_location.js
@@ +41,5 @@
+
+// Tests that console logging methods display the method location along with
+// the output in the console.
+
+const TEST_FILE_LOCATION_URI = "http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-bug-646025-console-file-location.html" + "?_date=" + Date.now();
Is the _date parameter needed for something?
Attachment #529726 -
Flags: feedback?(mihai.sucan) → feedback-
Assignee | ||
Comment 13•14 years ago
|
||
This updated patch adds a location check to the ConsoleAPI tests and removes the redundant cache busting _date parameter from the HUDService test.
Attachment #529726 -
Attachment is obsolete: true
Attachment #530002 -
Flags: feedback?(mihai.sucan)
Comment 14•14 years ago
|
||
Comment on attachment 530002 [details] [diff] [review]
Fourth version
Patch looks good! All tests pass. Feedback+ it is!
Now you can ask for a review. Good work!
Attachment #530002 -
Flags: feedback?(mihai.sucan) → feedback+
Assignee | ||
Comment 15•14 years ago
|
||
Comment on attachment 530002 [details] [diff] [review]
Fourth version
Requesting review from gavin, since he showed interest on IRC and no good deed ever goes unpunished.
Attachment #530002 -
Flags: review?(gavin.sharp)
Comment 16•14 years ago
|
||
I'm not sure I understand how this works - you're changing what ends up being passed to logConsoleAPIMessage as aArguments from an actual arguments array to an object that has a bunch of properties. Are you just relying on JSTerm's formatResult handling that properly? Doesn't it make the output a lot spammier?
Assignee | ||
Comment 17•14 years ago
|
||
(In reply to comment #16)
> I'm not sure I understand how this works - you're changing what ends up being
> passed to logConsoleAPIMessage as aArguments from an actual arguments array to
> an object that has a bunch of properties. Are you just relying on JSTerm's
> formatResult handling that properly? Doesn't it make the output a lot spammier?
Thanks for taking a look at this! Let me give you a brief overview of the change in the patch.
There is a difference currently in the structure of logConsoleAPIMessage's aArguments parameter, between the "trace" level and all the others. The main reason is that "trace" needs to communicate extra information about the call, like file and function name, and line number. In this bug we want to make sure the rest of the log levels have access to that same information, so my intent is to harmonize aArguments across all of them. And yes, formatResult handles it properly since there is no change from its POV. I would describe the new output not as spammier, but rather richer in information. Here is a screenshot, so that I don't spam you with 1000 more words :-)
http://astithas.com/images/bug-646025.png
Comment 18•14 years ago
|
||
Comment on attachment 530002 [details] [diff] [review]
Fourth version
Seems to me like the more compatible way to do this would be to add properties to the consoleEvent object that notifyObservers() sends out, rather than modifying its "arguments" property. That would mean changing notifyObservers's signature, but it wouldn't break any listeners of console-api-log-event that depended on the arguments being passed directly. It also means your changes to the output object can happen in ConsoleAPIObserver rather than in ConsoleAPI, which is more appropriate (ConsoleAPIObserver is the specific consumer you care to change here, ConsoleAPI is the general purpose notification).
(Ping me on IRC if I didn't explain that well)
Attachment #530002 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 19•14 years ago
|
||
I see your point and indeed it does simplify the patch a bit.
Attachment #530002 -
Attachment is obsolete: true
Attachment #531297 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Comment 20•14 years ago
|
||
Comment on attachment 531297 [details] [diff] [review]
Fifth version
>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>+ * @param string aFilename
>+ * The source file name reported by the console service.
>+ * @param string aFunctionName
>+ * The source function name reported by the console service.
>+ * @param string aLineNumber
>+ * The source file line number reported by the console service.
Perhaps it'd be better to pass these as properties on an aDetails object, just to avoid having so many parameters (particularly if we're going to be adding/removing properties in the future)? Or change this function so that it just takes the aMessage object directly...
r=me with that considered. Good test coverage!
Attachment #531297 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 21•14 years ago
|
||
Trimmed down the arguments list as advised and now we are passing the aMessage object directly. logConsoleAPIMessage already has deep knowledge of its internals anyway, so the increased coupling should not be a concern.
Attachment #531297 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [console] → [console][checkin-needed]
Reporter | ||
Updated•14 years ago
|
Whiteboard: [console][checkin-needed] → [console][fixed-in-devtools]
Reporter | ||
Comment 22•14 years ago
|
||
Comment on attachment 531575 [details] [diff] [review]
[checked-in][in-devtools] Final version
http://hg.mozilla.org/projects/devtools/rev/8e5852644b70
Attachment #531575 -
Attachment description: Final version → [in-devtools] Final version
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [console][fixed-in-devtools] → [console][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
Updated•14 years ago
|
Attachment #531575 -
Attachment description: [in-devtools] Final version → [checked-in][in-devtools] Final version
Comment 23•14 years ago
|
||
Comment on attachment 531575 [details] [diff] [review]
[checked-in][in-devtools] Final version
http://hg.mozilla.org/mozilla-central/rev/8e5852644b70
Comment 24•14 years ago
|
||
Verified fixed on:
Windows 7:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Window XP:
Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Mac OS 10.6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Linux i686:
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
*Note: If giving the input <console.log("this is some console output");> to the web console, the location of the method "Web Console:1" is displayed. Marking this as Verified.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•