As :mixedpuppy points out in bug 845546, Identity (LogUtils) and Downloads (DownloadsLogger) are using pretty much identical copies of the same code for logging; they should be consolidated.

The long term plan is for everyone to use log4moz (see bug 451283 for moving that to toolkit). Sqlite.jsm is already using it so it should be fine to switch toolkit/identity to use it as well (see bug 813833 comment 41).

Hi, Christian, Can I get your feedback on this logging module? Is it something you could use in lieu of the DownloadsLogger? My goals are to make something simple and unobtrusive that provides these features: - an easy way to give logs from related components the same signature - logging of caller's file, name, and line number - tracebacks for errors - an on/off switch using a pref Here's a usage example of the attached patch as entered in the scratchpad. It pretends to be a logger for an imaginary feature called 'lothar' whose logger can be toggled on using the pref 'toolkit.lothar.debug': XPCOMUtils.defineLazyGetter(this, "logger", function() { Cu.import("resource://gre/modules/logutils/Logger.jsm"); return getLogger('lothar', 'toolkit.lothar.debug'); }); function foo() { logger.log("Hi, mom!"); } foo(); // nothing happens; logging is disabled by pref Services.prefs.setBoolPref('toolkit.lothar.debug', true); logger.log("hello!"); foo(); The output to the console is: LogUtils enabled for lothar [lothar] Scratchpad/1 line 20: hello! [lothar] Scratchpad/1 foo() line 15: Hi, mom!

(In reply to Matthew N. [:MattN] from comment #1) > The long term plan is for everyone to use log4moz (see bug 451283 for moving > that to toolkit). Sqlite.jsm is already using it so it should be fine to > switch toolkit/identity to use it as well (see bug 813833 comment 41). Oh wow - that bug has been open for four years - that is a long-term plan! :) Thanks for the tip; I'll take a look at it.

I like what I see in log4moz for writing to files and having the good features of things like log4j and python logging, but this patch provides a different set of features that I find indispensable in debugging and developing, in particular: - reports filenames, function names, and line numbers of caller - can be enabled/disabled with a pref The stack introspection is obviously going to be an expensive operation, but that's why you would only opt in to use it. Maybe those features would have a place in log4moz? I don't know. But I feel like this is trying to solve a different set of problems in logging.

(In reply to Jed Parsons [:jparsons] from comment #2) > Can I get your feedback on this logging module? Is it something you could > use in lieu of the DownloadsLogger? It looks like it should work, but I don't know enough about log4moz to say whether or not those features ought instead to be introduced into there instead...

Hi, Dave, Do you think the functionality in this module would have a place in log4moz? If so, I'd be glad to help consolidate them.

(In reply to Jed Parsons [:jparsons] from comment #6) > Hi, Dave, > > Do you think the functionality in this module would have a place in log4moz? > If so, I'd be glad to help consolidate them. I think it'd be really useful to have it in there

Bug 451283 has now landed so this patch can probably be updated to build on top of it.

Bug 848569 - Replace DownloadsLogger with usage of ConsoleAPI. r=paolo

I think Console.jsm meets the needs of Download and Identity code now (after bug 1172141 and bug 1171298) so I don't think we need a new module.

(In reply to Matthew N. [:MattN] from comment #10) > I think Console.jsm meets the needs of Download and Identity code now (after > bug 1172141 and bug 1171298) so I don't think we need a new module. For the readinglist code, we split some of the sync logging code into its own module, services/common/logmanager. It's a somewhat messy helper for Log.jsm, but has the ability to generate log files (and clean up old ones) and various other things that make it suitable for use by Sync and readinglist. So I suspect "identity" related code, particularly when used around sync, is going to continue using that. The ability for users to capture and upload logs is extremely valuable in these contexts. This is just FYI - there's no implication that downloads should (or should not ;) use this module.

Comment on attachment 8617652 [details] MozReview Request: Bug 848569 - Replace DownloadsLogger with usage of ConsoleAPI. r=paolo https://reviewboard.mozilla.org/r/10677/#review9391 It's good to see more consistency in logging calls. Thanks! ::: browser/app/profile/firefox.js:360 (Diff revision 1) > -// Enable logging downloads operations to the Error Console. > -pref("browser.download.debug", false); > +// Enable logging downloads operations to the Console. > +pref("browser.download.debug", "Error"); Change the preference name to "browser.download.loglevel" both for consistency with the others and to avoid a type clash with a customized preference. ::: browser/components/downloads/DownloadsCommon.jsm:36 (Diff revision 1) > -const Cc = Components.classes; > +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components; nit: swap Cu and Cr, the most popular order is Cc, Ci, Cu, Cr. ::: browser/components/downloads/DownloadsCommon.jsm:65 (Diff revision 1) > XPCOMUtils.defineLazyModuleGetter(this, "DownloadsLogger", > "resource:///modules/DownloadsLogger.jsm"); Leftover module import. ::: browser/components/downloads/DownloadsCommon.jsm (Diff revision 1) > - error(...aMessageArgs) { We have still one caller of "error". Either replace it with "Cu.reportError" (it's an exception code path), or keep the function.

https://reviewboard.mozilla.org/r/10677/#review9469 > Change the preference name to "browser.download.loglevel" both for consistency with the others and to avoid a type clash with a customized preference. Note that I tested that the change of type would have been handled fine. The user set value would have been discarded. I've renamed it for clarity.

Comment on attachment 8617652 [details] MozReview Request: Bug 848569 - Replace DownloadsLogger with usage of ConsoleAPI. r=paolo Bug 848569 - Replace DownloadsLogger with usage of ConsoleAPI. r=paolo

Comment on attachment 8617652 [details] MozReview Request: Bug 848569 - Replace DownloadsLogger with usage of ConsoleAPI. r=paolo https://reviewboard.mozilla.org/r/10677/#review9491 Ship It!

https://reviewboard.mozilla.org/r/10677/#review9489 > Note that I tested that the change of type would have been handled fine. The user set value would have been discarded. I've renamed it for clarity. Nice to see our preference handling code does the right thing!