Closed
Bug 475033
Opened 16 years ago
Closed 16 years ago
log4moz utility function to provide preconfigured loggers
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: davida, Assigned: davida)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
asuth
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
As per discussion on IRC, here's a patch to make it easy to use log4moz with reasonable default loggers, and reasonable conventions for how to use prefs to configure those defaults w/o code changes.
Assuming that given that log4moz is in the gloda dir, asuth is the right reviewer.
I'm using this in an upcoming revision of the patch for 257942 (activity manager), which blocks b2, so prompt review would be great.
Attachment #358425 -
Flags: review?(bugmail)
Comment 1•16 years ago
|
||
Comment on attachment 358425 [details] [diff] [review]
log4moz patch
For the doxygen, one might ideally use the @param tags. The counterpoint is that we don't really have anything that consumes them anyways.
>+ let consoleLevel = consoleLevel || -1;
>+ let dumpLevel = dumpLevel || -1;
nit: Since these are already arguments, I'm not sure you need to declare them with a let.
>+ let prefService = Components.classes["@mozilla.org/preferences-service;1"].
>+ getService(Components.interfaces.nsIPrefService);
You can use Cc and Ci here.
Attachment #358425 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 2•16 years ago
|
||
Thanks. Updated patch attached. carrying forward r+.
checkin at will.
Attachment #358425 -
Attachment is obsolete: true
Attachment #358545 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 3•16 years ago
|
||
I tried |hg push will| but it didn't work very well, so I went with http://hg.mozilla.org/comm-central/rev/d2a0b60db72b instead.
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: unspecified → Trunk
Assignee | ||
Comment 4•16 years ago
|
||
Oops. Turns out string prefs are looked up with getCharPref, not getStringPref. So the pref part never worked. The exception handling (needed because there's no "hasCharPref" call masked the exception. This patch fixes the bug and contains the exception handling more, to avoid similar snafus in the future.
Attachment #358545 -
Attachment is obsolete: true
Attachment #359648 -
Flags: review?(bugmail)
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•16 years ago
|
Attachment #359648 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 5•16 years ago
|
||
This is a better fix that also makes sure that w/ no arguments to getConfiguredLogger, we end up with loggers that will log errors & above, which is sort of the point.
Attachment #359648 -
Attachment is obsolete: true
Attachment #360383 -
Flags: review?(bugmail)
Comment 6•16 years ago
|
||
Comment on attachment 360383 [details] [diff] [review]
better fix
So, it'll break if the preference value is not a case-variant of 'none' or one of the legal options. (Undefined gets in there and ends up getting set.) But I'm not sure there's really a good way to handle that case anyways.
Attachment #360383 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 7•16 years ago
|
||
Bienvenu, this patch makes debugging the autosync stuff easier. it'd be good to get this in b2.
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #360383 -
Flags: superreview+
Comment 8•16 years ago
|
||
Comment on attachment 360383 [details] [diff] [review]
better fix
I'm going to interpret that last comment as a request for sr :-)
sr=bienvenu with a couple nits:
No need for the temp prefService var here:
+ let prefService = Cc["@mozilla.org/preferences-service;1"].
+ getService(Ci.nsIPrefService);
+ branch = prefService.getBranch(loggername + ".logging.");
We could use the ? operator here for consoleLevel and dumpLevel
let me know if you want me to drive this in for b2 or if someone else will take it in.
Comment 10•16 years ago
|
||
fix checked in
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•