Control MozLoopService logging level with a preference

RESOLVED FIXED in Firefox 34

Status

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

unspecified
mozilla35
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

(Whiteboard: [loop-uplift])

Attachments

(1 attachment, 1 obsolete attachment)

Being able to control the log level with a preference allows developers to keep more logging in the code without flooding the Browser Console for non-Loop developers.

From now on, logging in MozLoopService should use log.* instead of console.* methods.
Attachment #8495342 - Flags: review?(jaws)
Flags: qe-verify-
Flags: firefox-backlog+
Remove leftover unused import of Log.jsm.
Attachment #8495342 - Attachment is obsolete: true
Attachment #8495342 - Flags: review?(jaws)
Attachment #8495346 - Flags: review?(jaws)
Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

Review of attachment 8495346 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +70,5 @@
> +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> +XPCOMUtils.defineLazyGetter(this, "log", () => {
> +  let ConsoleAPI = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).ConsoleAPI;
> +  let level = Services.prefs.getPrefType(PREF_LOG_LEVEL) == Ci.nsIPrefBranch.PREF_STRING &&
> +                Services.prefs.getCharPref(PREF_LOG_LEVEL);

Why do we need to do a check here to make sure that this value is a string? There are plenty of places in our codebase that we call getCharPref before checking that the type of the preference is a string.

@@ +1244,5 @@
>    setLoopCharPref: function(prefName, value) {
>      try {
>        Services.prefs.setCharPref("loop." + prefName, value);
>      } catch (ex) {
> +      log.error("setLoopCharPref had trouble setting " + prefName +

This is a change from console.log (info) to log.error, which, given the duties of this code seems right. We shouldn't be failing to set a pref, and if we do we should log that prominently. But I just wanted to point this out in case you did this on accident.
Attachment #8495346 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8495346 [details] [diff] [review]
> v.1.1 Use maxLogLevel of ConsoleAPI.
> 
> Review of attachment 8495346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +70,5 @@
> > +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> > +XPCOMUtils.defineLazyGetter(this, "log", () => {
> > +  let ConsoleAPI = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).ConsoleAPI;
> > +  let level = Services.prefs.getPrefType(PREF_LOG_LEVEL) == Ci.nsIPrefBranch.PREF_STRING &&
> > +                Services.prefs.getCharPref(PREF_LOG_LEVEL);
> 
> Why do we need to do a check here to make sure that this value is a string?
> There are plenty of places in our codebase that we call getCharPref before
> checking that the type of the preference is a string.

That was because I was considering making it a hidden pref like some other modules did so I didn't want to get an exception if the pref wasn't defined. I decided at least while loop is in active development it makes sense to make the pref visible to make it easier for devs and QE to adjust it. I'll remove the getPrefType stuff.

> @@ +1244,5 @@
> >    setLoopCharPref: function(prefName, value) {
> >      try {
> >        Services.prefs.setCharPref("loop." + prefName, value);
> >      } catch (ex) {
> > +      log.error("setLoopCharPref had trouble setting " + prefName +
> 
> This is a change from console.log (info) to log.error, which, given the
> duties of this code seems right. We shouldn't be failing to set a pref, and
> if we do we should log that prominently. But I just wanted to point this out
> in case you did this on accident.

Yeah, that was intentional so that it shows up at the default log level and because it's an error. I think this can happen if the type of the value is not valid for that set*Pref function. e.g. only letters passed to setIntPref.
https://hg.mozilla.org/integration/fx-team/rev/f5a053aaba52

Thanks
Flags: in-testsuite-
Whiteboard: [loop-uplift] → [loop-uplift][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f5a053aaba52
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [loop-uplift][fixed-in-fx-team] → [loop-uplift]
Target Milestone: --- → mozilla35
Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8495346 - Flags: approval-mozilla-aurora?
Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8495346 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.