For compatibility with Log.jsm (in the event that they are unified one day), I've always recommended using upper-case first log levels since they work with Console.jsm and Log.jsm: * Console.jsm is case-insensitive for maxLogLevel[1] * Log.jsm requires an uppercase first letter console.createInstance isn't compatible with an uppercase first letter as the `ConsoleLogLevel` enum uses lowercase. I think we should change `ConsoleLogLevel` to be uppercase-first to match Log.jsm. The only pref I see that wasn't following this is dom.push.loglevel=error so it's much easier to change that one place and devs who set it rather than changing all the other consumers and other dev profiles. [1] https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/toolkit/modules/Console.jsm#617

It would also be great if we had a warning in non-debug builds for when the pref value is set to an invalid value so that we can catch these problems easier.

Comment on attachment 8960474 [details] [diff] [review] part 1 - Upper-case Level Review of attachment 8960474 [details] [diff] [review]: ----------------------------------------------------------------- Can you also update the casing for the push logLevel pref in this changeset? https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/modules/libpref/init/all.js#5223

Comment on attachment 8960473 [details] [diff] [review] part 2 - Warning message Review of attachment 8960473 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/console/ConsoleInstance.cpp @@ +34,5 @@ > > nsAutoCString value; > nsresult rv = Preferences::GetCString(aPref.get(), value); > if (NS_WARN_IF(NS_FAILED(rv))) { > +#ifdef DEBUG Is it possible to report and error to the browser console instead? NS_WARNING is better than nothing but people in non debug builds or on windows this will be hard to find. If it's not easy or there would be some side effects due to this running in console instance creation then don't worry about it. @@ +46,5 @@ > > int index = FindEnumStringIndexImpl(value.get(), value.Length(), > ConsoleLogLevelValues::strings); > if (NS_WARN_IF(index < 0)) { > return ConsoleLogLevel::All; Can you also log in this condition (which is what would get hit when the casing is wrong)?

Comment on attachment 8961719 [details] [diff] [review] part 2 - Warning message Review of attachment 8961719 [details] [diff] [review]: ----------------------------------------------------------------- Thanks

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a19ca99d150 Upper-case log levels for Console.createInstance(), r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/2acdab792926 Warning messages if console level pref doesn't exist, r=bgrins

https://hg.mozilla.org/mozilla-central/rev/5a19ca99d150 https://hg.mozilla.org/mozilla-central/rev/2acdab792926