1447210 - console.createInstance requires lowercase log levels while Console.jsm was case-insensitive

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Matthew N. [:MattN]]

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 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Warning message

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - Upper-case Level

Comment 4

[Brian Grinstead [:bgrins]]

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 5

[Brian Grinstead [:bgrins]]

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 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - Warning message

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8961719 [details] [diff] [review]
part 2 - Warning message

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

Thanks

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Cristina Coroiu [:ccoroiu]]

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