Closed Bug 646025 Opened 13 years ago Closed 13 years ago

Add file location to console.log, info, debug, etc.

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 6

People

(Reporter: rcampbell, Assigned: past)

Details

(Whiteboard: [console][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 file, 5 obsolete files)

[checked-in][in-devtools] Final version
18.49 KB, 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
Whiteboard: [console]
Assignee: nobody → past
Attached patch First patch attempt (obsolete) — — Splinter Review 

        Attachment #529445 -
        Flags: feedback?(mihai.sucan)

    
    

    
  
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-

    
    

    
  
(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?

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
Updated patch after incorporating feedback.

        Attachment #529445 -
        Attachment is obsolete: true

        Attachment #529672 -
        Flags: feedback?(mihai.sucan)

    
    

    
  
(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!

    
    

    
  
(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 on attachment 529672 [details] [diff] [review]
Updated patch

Asking gerv for feedback too, then.

gerv: See comment 3 above.

        Attachment #529672 -
        Flags: feedback?(gerv)

    
    

    
  
(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

    
    

    
  
Nope, you've been crystal clear :-)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Third version of the patch (obsolete)
        — 
          — Splinter Review
      

    


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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fourth version (obsolete)
        — 
          — Splinter Review
      

    


  
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 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+

    
    

    
  
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)

    
    

    
  
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?

    
    

    
  
(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 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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fifth version (obsolete)
        — 
          — Splinter Review
      

    


  
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)
Status: NEW → ASSIGNED

    
    

    
  
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+

    
    

    
  


  
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
Whiteboard: [console] → [console][checkin-needed]

    
  
Whiteboard: [console][checkin-needed] → [console][fixed-in-devtools]

    
    

    
  
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

    
  
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [console][fixed-in-devtools] → [console][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6

    
  

        Attachment #531575 -
        Attachment description: [in-devtools] Final version → [checked-in][in-devtools] Final version

    
    

    
  
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

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: