Closed Bug 1447210 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: MattN, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(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.

[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.
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? https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/modules/libpref/init/all.js#5223
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]:
-----------------------------------------------------------------

Thanks
Attachment #8961719 - Flags: review?(bgrinstead) → review+
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
Status: NEW → RESOLVED
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.