379797 - dump() in JS Components should obey browser.dom.window.dump.enabled

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Daniel Veditz [:dveditz]]

Inspired by problems such as regression bug 379722:

In DOM code dump() does not print anything in non-debug builds unless you set dom.window.dump.enabled to true. Developers feel free to sprinkle dump() and not worry too much.

In JS components, however, dump() always dumps which can result in debug code ending up in shipping builds. What the developers intended was no doubt debug(), but having different behavior for the same function is confusing.

Various options:
 - make dump() synonymous with debug() and add a print()
 - make dump() obey the pref. Currently xpconnect/loader does not
   depend on prefs, but it REQUIRES caps and necko which do
 - in DEBUG builds have dump(<foo>) spit out
   "**Did you mean to use debug() instead?**\n<foo>"
   (only sort of kidding)
 - ?something else?

Comment 1

[James Ross]

From a developer perspective, I believe following the pref (and thus making it behave the same as window.dump()) would be the most logical. (Of course, the pref then has a silly name for what it does, but you can't have everything.)

Comment 2

[Andrew McCreight [:mccr8]]

I tweeted about bug 1386036, and mconley mentioned the pref browser.dom.window.dump.enabled. I tracked down where the dump() was implemented, which eventually led me to this bug. I'll just change this dump method to obey the pref.

Comment 3

[Andrew McCreight [:mccr8]]

Sandboxes have yet another implementation of dump(), so I'll fix that, too.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 379797 - Various dump() methods should check browser.dom.window.dump.enabled.

Checking this pref to avoid log spam in opt builds, in sandboxes, JS
components, and whatever uses nsFrameMessageManager's dump method.

This does mean that on Windows in an opt build when a debugger is
present a debug string will no longer be printed unless the pref is
set, but I think that is consistent with the non-Windows behavior.

Review commit: https://reviewboard.mozilla.org/r/164042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164042/

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8893055 [details]
Bug 379797 - Various dump() methods should check browser.dom.window.dump.enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164042/diff/1-2/

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8893055 [details]
Bug 379797 - Various dump() methods should check browser.dom.window.dump.enabled.

https://reviewboard.mozilla.org/r/164042/#review170242

\o/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/693f52e6fb26
Various dump() methods should check browser.dom.window.dump.enabled. r=krizsa

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/693f52e6fb26