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

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Matthew N. [:MattN]]

Attachment: 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 2

[Brian Grinstead [: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/#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?

Comment 3

[Matthew N. [:MattN]]

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 4

[Matthew N. [:MattN]]

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 5

[Matthew N. [:MattN]]

Attachment: 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 6

[Matthew N. [:MattN]]

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 7

[Matthew N. [:MattN]]

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 8

[Brian Grinstead [: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!

Comment 9

[Brian Grinstead [: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

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

Ship It!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7584dcefd15c
https://hg.mozilla.org/integration/fx-team/rev/84d422afe66d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7584dcefd15c
https://hg.mozilla.org/mozilla-central/rev/84d422afe66d