Last Comment Bug 646025 - Add file location to console.log, info, debug, etc.
: Add file location to console.log, info, debug, etc.
Status: VERIFIED FIXED
[console][fixed-in-devtools][merged-t...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Panos Astithas [:past]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-29 05:47 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-05-26 05:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch attempt (9.59 KB, patch)
2011-05-02 04:08 PDT, Panos Astithas [:past]
mihai.sucan: feedback-
Details | Diff | Splinter Review
Updated patch (9.27 KB, patch)
2011-05-03 04:05 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Third version of the patch (11.04 KB, patch)
2011-05-03 08:32 PDT, Panos Astithas [:past]
mihai.sucan: feedback-
Details | Diff | Splinter Review
Fourth version (16.95 KB, patch)
2011-05-04 07:41 PDT, Panos Astithas [:past]
gavin.sharp: review-
mihai.sucan: feedback+
Details | Diff | Splinter Review
Fifth version (16.72 KB, patch)
2011-05-10 05:01 PDT, Panos Astithas [:past]
gavin.sharp: review+
Details | Diff | Splinter Review
[checked-in][in-devtools] Final version (18.49 KB, patch)
2011-05-11 03:07 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-03-29 05:47:11 PDT
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
Comment 1 Panos Astithas [:past] 2011-05-02 04:08:48 PDT
Created attachment 529445 [details] [diff] [review]
First patch attempt
Comment 2 Mihai Sucan [:msucan] 2011-05-02 11:05:15 PDT
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.
Comment 3 Panos Astithas [:past] 2011-05-03 01:36:26 PDT
(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 Mihai Sucan [:msucan] 2011-05-03 02:04:38 PDT
(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.
Comment 5 Panos Astithas [:past] 2011-05-03 04:05:47 PDT
Created attachment 529672 [details] [diff] [review]
Updated patch

Updated patch after incorporating feedback.
Comment 6 Panos Astithas [:past] 2011-05-03 04:13:05 PDT
(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 Shawn Wilsher :sdwilsh 2011-05-03 07:26:09 PDT
(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.
Comment 8 Panos Astithas [:past] 2011-05-03 07:47:03 PDT
Comment on attachment 529672 [details] [diff] [review]
Updated patch

Asking gerv for feedback too, then.

gerv: See comment 3 above.
Comment 9 Gervase Markham [:gerv] 2011-05-03 08:19:17 PDT
(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
Comment 10 Panos Astithas [:past] 2011-05-03 08:22:06 PDT
Nope, you've been crystal clear :-)
Comment 11 Panos Astithas [:past] 2011-05-03 08:32:08 PDT
Created attachment 529726 [details] [diff] [review]
Third version of the patch

Restored the initial license text for the test file per gerv's comment and rebased after the last merge from m-c.
Comment 12 Mihai Sucan [:msucan] 2011-05-03 08:55:44 PDT
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?
Comment 13 Panos Astithas [:past] 2011-05-04 07:41:52 PDT
Created attachment 530002 [details] [diff] [review]
Fourth version

This updated patch adds a location check to the ConsoleAPI tests and removes the redundant cache busting _date parameter from the HUDService test.
Comment 14 Mihai Sucan [:msucan] 2011-05-05 09:54:48 PDT
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!
Comment 15 Panos Astithas [:past] 2011-05-05 14:06:35 PDT
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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-05 14:17:04 PDT
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?
Comment 17 Panos Astithas [:past] 2011-05-06 03:01:01 PDT
(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 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-09 17:26:09 PDT
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)
Comment 19 Panos Astithas [:past] 2011-05-10 05:01:56 PDT
Created attachment 531297 [details] [diff] [review]
Fifth version

I see your point and indeed it does simplify the patch a bit.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-10 10:46:23 PDT
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!
Comment 21 Panos Astithas [:past] 2011-05-11 03:07:15 PDT
Created attachment 531575 [details] [diff] [review]
[checked-in][in-devtools] Final version

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.
Comment 22 Rob Campbell [:rc] (:robcee) 2011-05-12 05:32:14 PDT
Comment on attachment 531575 [details] [diff] [review]
[checked-in][in-devtools] Final version

http://hg.mozilla.org/projects/devtools/rev/8e5852644b70
Comment 23 Dave Camp (:dcamp) 2011-05-21 21:16:05 PDT
Comment on attachment 531575 [details] [diff] [review]
[checked-in][in-devtools] Final version

http://hg.mozilla.org/mozilla-central/rev/8e5852644b70
Comment 24 AndreiD[QA] 2011-05-26 05:53:29 PDT
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.

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