Closed Bug 1141621 Opened 10 years ago Closed 9 years ago

Put shumway logging behind a pref

Categories

(Firefox Graveyard :: Shumway, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: mossop, Assigned: MattN)

References

Details

Attachments

(1 file, 1 obsolete file)

We've had a general policy in the past that any logging in code should be controlled by a pref, default off unless there are particular reasons. This keeps console output as clean as possible for people running and debugging tests locally. Can we put Shumway's logging behind a pref? http://mxr.mozilla.org/mozilla-central/source/browser/extensions/shumway/chrome/ShumwayCom.jsm#49 http://mxr.mozilla.org/mozilla-central/source/browser/extensions/shumway/content/ShumwayUtils.jsm#38 http://mxr.mozilla.org/mozilla-central/source/browser/extensions/shumway/content/ShumwayStreamConverter.jsm#51
Flags: needinfo?(ydelendik)
Can we perhaps use shumway.debug.enabled for that?
Sounds good to me. You might also find it easier to use Preferences.jsm for prefs access rather than Services.prefs as you can define default values rather than needing try...catch blocks.
A logging pref doesn't need to block M3, our initial Nightly testing, because Shumway logging won't be as common because our whitelist will be pretty short.
Blocks: shumway-m4
(In reply to Chris Peterson [:cpeterson] from comment #3) > A logging pref doesn't need to block M3, our initial Nightly testing, > because Shumway logging won't be as common because our whitelist will be > pretty short. The logging in ShumwayUtils.jsm happens regardless of the size of the whitelist and happens on startup and shutdown which means it appears in all test logs. Someone else can deal with the other logging which isn't as prevalent.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Attachment #8612635 - Flags: review?(cpeterson)
Comment on attachment 8612635 [details] [diff] [review] v.1 Put ShumwayUtils.jsm logging behind shumway.debug Review of attachment 8612635 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Matt. The Shumway code in browser/extensions/shumway/ is actually a read-only copy of Shumway's upstream repo https://github.com/mozilla/shumway/. Yury, can you please commit Matt's logging change (or something equivalent) before you land the next Shumway build on mozilla-central? I think these Shumway messages may be annoying other developers. :)
Attachment #8612635 - Flags: review?(cpeterson) → feedback?(ydelendik)
Yury, can you pick this up?
Sending to Tobias because Yury is on PTO. :)
Flags: needinfo?(ydelendik) → needinfo?(schneider)
Comment on attachment 8612635 [details] [diff] [review] v.1 Put ShumwayUtils.jsm logging behind shumway.debug Review of attachment 8612635 [details] [diff] [review]: ----------------------------------------------------------------- shumway.debug.enabled can be used for that.
Attachment #8612635 - Flags: feedback?(ydelendik) → feedback+
(In reply to Yury Delendik (:yury) from comment #8) > Comment on attachment 8612635 [details] [diff] [review] > v.1 Put ShumwayUtils.jsm logging behind shumway.debug > > Review of attachment 8612635 [details] [diff] [review]: > ----------------------------------------------------------------- > > shumway.debug.enabled can be used for that. Are you going to make the pref name change and land this on GitHub? Or what are the next steps here?
You can change the pref on m-c, but open an issue at github to follow up.
Also switched to single quotes to match the file.
Attachment #8612635 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Attachment #8715482 - Flags: review?(ydelendik)
Product: Firefox → Firefox Graveyard
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → INCOMPLETE
Comment on attachment 8715482 [details] [diff] [review] v.2 Put ShumwayUtils.jsm logging behind shumway.debug.enabled Review of attachment 8715482 [details] [diff] [review]: ----------------------------------------------------------------- The Shumway code was removed from the m-c.
Attachment #8715482 - Flags: review?(ydelendik)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: