Closed Bug 1447210 Opened 2 years ago Closed 2 years ago

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


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




Tracking Status
firefox61 --- fixed


(Reporter: MattN, Assigned: baku)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

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.

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.
Attached patch part 2 - Warning message (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8960473 - Flags: review?(bgrinstead)
Attachment #8960474 - Flags: review?(bgrinstead)
Priority: -- → P2
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?
Attachment #8960474 - Flags: review?(bgrinstead) → review+
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)?
Attachment #8960473 - Flags: review?(bgrinstead)
Attachment #8960473 - Attachment is obsolete: true
Attachment #8961719 - Flags: review?(bgrinstead)
Comment on attachment 8961719 [details] [diff] [review]
part 2 - Warning message

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

Attachment #8961719 - Flags: review?(bgrinstead) → review+
Pushed by
Upper-case log levels for Console.createInstance(), r=bgrins
Warning messages if console level pref doesn't exist, r=bgrins
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.