Closed Bug 475033 Opened 15 years ago Closed 15 years ago

log4moz utility function to provide preconfigured loggers

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

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)

Attached patch log4moz patch (obsolete) — 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 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+
Attached patch Revised patch (obsolete) — Splinter Review
Thanks.  Updated patch attached.  carrying forward r+.

checkin at will.
Attachment #358425 - Attachment is obsolete: true
Attachment #358545 - Flags: review+
Keywords: checkin-needed
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: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: unspecified → Trunk
Attached patch fix (obsolete) — Splinter Review
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #359648 - Flags: review?(bugmail) → review+
Attached patch better fixSplinter Review
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 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+
Bienvenu, this patch makes debugging the autosync stuff easier.  it'd be good to get this in b2.
Keywords: checkin-needed
Attachment #360383 - Flags: superreview+
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.
ok, I'll fix the nits myself and land this.
Keywords: checkin-needed
fix checked in
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.