Make it simple to construct a Console with a live preference for the maxLogLevel

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

unspecified
Firefox 41

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

It's a common pattern to want to want to create an instance of the ConsoleAPI with a prefix related to the module and with the maxLogLevel controlled by a live preference. We should make this possible with a single line (excluding the import) to reduce boilerplate and consistency.

I've implemented this pattern in a few different modules already:
* https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.jsm?rev=438e73cdb242&mark=41-61#41
* https://mxr.mozilla.org/mozilla-central/source/browser/components/loop/modules/MozLoopService.jsm?rev=bc4fc60d5759&mark=121-129#121
* https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm?rev=3f934d7672b3&mark=69-78#69

I think it's time to bake this into Console.jsm
Created attachment 8616273 [details]
MozReview Request: Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins

Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins
Attachment #8616273 - Flags: review?(bgrinstead)
Comment on attachment 8616273 [details]
MozReview Request: Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins

https://reviewboard.mozilla.org/r/10447/#review9209

::: browser/devtools/webconsole/test/browser_console_consolejsm_output.js:228
(Diff revision 1)
> +  Services.prefs.setCharPref(consoleOptions.maxLogLevelPref, "warn");

We should set this to an uppercase string at some point (maybe here) and then assert that consoleOptions.maxLogLevel has been lowercased just to exercise that functionality in the test

::: toolkit/devtools/Console.jsm:612
(Diff revision 1)
> +      this.maxLogLevel = aConsoleOptions.maxLogLevel || "all";

There is a sort of subtle case where maxLogLevel may end up as something you don't expect, which is:

  let console = new ConsoleAPI({
    maxLogLevel: "error",
    maxLogLevelPref: "testing.maxLogLevel",
  });
  
  console.maxLogLevel = "warn";
  Services.prefs.clearUserPref(consoleOptions.maxLogLevelPref);
  console.maxLogLevel; // "error"
 
I could imagine a potential problem if you did something like:

  let console = new ConsoleAPI({
    maxLogLevelPref: "testing.maxLogLevel",
  });
  
  #ifdef RELEASE_BUILD
  console.maxLogLevel = "error";
  #endif
  
In that case, the log would end up noisy by default (even in a release build) if a pref wasn't defined.

The alternative would be to do nothing if the maxLogLevelPref is not a string.  If this were the case, then if the pref were removed the maxLogLevel value would just remain what it was before the removal.  Which could cause some sort of surprising behavior in that case if you were wanting to prevent logs from showing up by removing a pref.  It's not 100% clear to me what the right behavior is in this case, but it feels safer to default to make the 'weird' case happen to people who are fiddling around with prefs and just use the previously set maxLogLevel.  They could always set the pref to 'none' if they want the logs to go away.  In other words, remove this condition.  What do you think?
Attachment #8616273 - Flags: review?(bgrinstead)
https://reviewboard.mozilla.org/r/10447/#review9241

> There is a sort of subtle case where maxLogLevel may end up as something you don't expect, which is:
> 
>   let console = new ConsoleAPI({
>     maxLogLevel: "error",
>     maxLogLevelPref: "testing.maxLogLevel",
>   });
>   
>   console.maxLogLevel = "warn";
>   Services.prefs.clearUserPref(consoleOptions.maxLogLevelPref);
>   console.maxLogLevel; // "error"
>  
> I could imagine a potential problem if you did something like:
> 
>   let console = new ConsoleAPI({
>     maxLogLevelPref: "testing.maxLogLevel",
>   });
>   
>   #ifdef RELEASE_BUILD
>   console.maxLogLevel = "error";
>   #endif
>   
> In that case, the log would end up noisy by default (even in a release build) if a pref wasn't defined.
> 
> The alternative would be to do nothing if the maxLogLevelPref is not a string.  If this were the case, then if the pref were removed the maxLogLevel value would just remain what it was before the removal.  Which could cause some sort of surprising behavior in that case if you were wanting to prevent logs from showing up by removing a pref.  It's not 100% clear to me what the right behavior is in this case, but it feels safer to default to make the 'weird' case happen to people who are fiddling around with prefs and just use the previously set maxLogLevel.  They could always set the pref to 'none' if they want the logs to go away.  In other words, remove this condition.  What do you think?

In the second example I think maxLogLevel would be "error" in release builds.
Comment on attachment 8616273 [details]
MozReview Request: Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins

Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins
Attachment #8616273 - Flags: review?(bgrinstead)
Created attachment 8616961 [details]
MozReview Request: Bug 1172141 - Use the maxLogLevelPref ConsoleAPI option for Loop and UITour to make logging preferences live. r=bgrins

Bug 1172141 - Use the maxLogLevelPref ConsoleAPI option for Loop and UITour to make logging preferences live. r=bgrins
Attachment #8616961 - Flags: review?(bgrinstead)
Comment on attachment 8616273 [details]
MozReview Request: Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins

Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins
Comment on attachment 8616961 [details]
MozReview Request: Bug 1172141 - Use the maxLogLevelPref ConsoleAPI option for Loop and UITour to make logging preferences live. r=bgrins

Bug 1172141 - Use the maxLogLevelPref ConsoleAPI option for Loop and UITour to make logging preferences live. r=bgrins
Comment on attachment 8616273 [details]
MozReview Request: Bug 1172141 - Add a maxLogLevelPref option to the ConsoleAPI constructor to easily control log levels with a preference. r=bgrins

https://reviewboard.mozilla.org/r/10447/#review9251

Ship It!
Attachment #8616273 - Flags: review?(bgrinstead) → review+
Comment on attachment 8616961 [details]
MozReview Request: Bug 1172141 - Use the maxLogLevelPref ConsoleAPI option for Loop and UITour to make logging preferences live. r=bgrins

https://reviewboard.mozilla.org/r/10491/#review9253

Ship It!
Attachment #8616961 - Flags: review?(bgrinstead) → review+
See Also: → bug 1171927
Summary: Make it simple to construct a Console with a prefix and live preference for the maxLogLevel → Make it simple to construct a Console with a live preference for the maxLogLevel
https://hg.mozilla.org/mozilla-central/rev/7584dcefd15c
https://hg.mozilla.org/mozilla-central/rev/84d422afe66d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.